Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Snapshot implementation #135

Merged
merged 11 commits into from
May 14, 2020
Merged

Snapshot implementation #135

merged 11 commits into from
May 14, 2020

Conversation

dabr-grapeup
Copy link
Contributor

Issue #, if available: #45

Description of changes:
Action for creating snapshot with two steps: attempt_snapshot and wait_for_snapshot

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@dbbaughe
Copy link
Contributor

Pulled it down to check, seems like the createRepository(repository) in the snapshot IT test might be causing the issue or part of it.
Once I uncommented that line and ran the integration tests all the failures happened.

@dabr-grapeup
Snapshot implementation
08cd736 
Action has two steps:
attempt_snapshot and wait_for_snapshot
@dbbaughe
Copy link
Contributor

Hi @dabr-grapeup,

Just a quicks heads-up, will be able to take a look at this towards the end of this week.
Currently getting things ready for the ODFE 1.4 release.

Thanks,
Drew

dbbaughe
dbbaughe reviewed Feb 5, 2020
View reviewed changes
Copy link
Contributor

@dbbaughe dbbaughe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @dabr-grapeup,

Sorry for the late feedback, will take a look at the tests and wait for portion soon, left some comments in the meantime.

...n/kotlin/com/amazon/opendistroforelasticsearch/indexstatemanagement/action/SnapshotAction.kt Show resolved Hide resolved
.../amazon/opendistroforelasticsearch/indexstatemanagement/model/action/SnapshotActionConfig.kt Show resolved Hide resolved
.../amazon/opendistroforelasticsearch/indexstatemanagement/step/snapshot/AttemptSnapshotStep.kt Show resolved Hide resolved
.../amazon/opendistroforelasticsearch/indexstatemanagement/step/snapshot/WaitForSnapshotStep.kt Show resolved Hide resolved
.../amazon/opendistroforelasticsearch/indexstatemanagement/step/snapshot/AttemptSnapshotStep.kt Outdated Show resolved Hide resolved
.../amazon/opendistroforelasticsearch/indexstatemanagement/step/snapshot/AttemptSnapshotStep.kt Outdated Show resolved Hide resolved
.../amazon/opendistroforelasticsearch/indexstatemanagement/step/snapshot/AttemptSnapshotStep.kt Show resolved Hide resolved
.../amazon/opendistroforelasticsearch/indexstatemanagement/step/snapshot/AttemptSnapshotStep.kt Show resolved Hide resolved
.../amazon/opendistroforelasticsearch/indexstatemanagement/step/snapshot/AttemptSnapshotStep.kt Show resolved Hide resolved
.../amazon/opendistroforelasticsearch/indexstatemanagement/step/snapshot/AttemptSnapshotStep.kt Outdated Show resolved Hide resolved
@dbbaughe dbbaughe mentioned this pull request Feb 5, 2020
Open
Daniel Bryla and others added 6 commits April 8, 2020 09:46
Add timestamp to snapshot
f9f9d56
@JavierAbrego
added missing license headers
bf3690c 
replaced LinkedHashMap in SnapshotAction
handle waitForSnapshotStep is not completed
added usermetadata
added hash at the end of the name
set explicit waitForCompletion
using CONDITION_NOT_MET
@JavierAbrego
Merge branch 'feature/opendistro-for-elasticsearch#45-snapshot-change…
afb1025 
…s' into opendistro-for-elasticsearch#45-snapshot

# Conflicts:
#	src/main/kotlin/com/amazon/opendistroforelasticsearch/indexstatemanagement/step/snapshot/AttemptSnapshotStep.kt
@JavierAbrego
added snapshot name hash
c872382
@JavierAbrego
Merge branch 'master' of https://github.com/opendistro-for-elasticsea…
208fd54 
…rch/index-management into opendistro-for-elasticsearch#45-snapshot
@JavierAbrego
implemented isIdempotent method
f76abd2
@JavierAbrego
Copy link
Contributor

Hi @dbbaugh, thanks for your comments, we have fixed some of the issues that you commented, let me know if you spot something else.

Thanks

@dabr-grapeup dabr-grapeup force-pushed the #45-snapshot branch 3 times, most recently from 1538ece to ffbfe0f Compare April 20, 2020 09:53
@dabr-grapeup
Do not fail snapshot if another one is in progress
3c14c66 
+ Restore snapshot name with timestamp
@dbbaughe
Copy link
Contributor

Thanks @dabr-grapeup and @JavierAbrego,

Will pull down and take another look at it. The main thing I see from GitHub is the global state parameter which I think we'll need to remove. But, let me confirm it has the negative effect we're trying to avoid.

@jinsoor-amzn @qreshi Can one of you also take a look as second reviewer

@alolita
Copy link

alolita commented May 12, 2020

Hi @dabr-grapeup @JavierAbrego - There are some changes our reviewers have requested. Could you please take a look and make the requested changes so that we can merge. Please let us know if you have any questions! Thanks for your patience on this PR :-)

@JavierAbrego
Copy link
Contributor

Hello @alolita @jinsoor-amzn @dbbaughe, thanks for your comments, I've pushed the changes.
Let me know if you spot something else : )

@dbbaughe
Copy link
Contributor

Hey @JavierAbrego,

Thanks for the quick update, I believe there was just one comment missed here. I tested the include_global_state option and it's not something we can include in this first release as it doesn't work well with ISM's metadata in the cluster state. We'll need to address it in a follow up release. Should be able to just remove the logic where it's added to the create snapshot request. Once we get that removed we should be able to merge.

@JavierAbrego
Copy link
Contributor

Sure @dbbaughe I just removed the logic. 👍

dbbaughe
dbbaughe approved these changes May 14, 2020
View reviewed changes
Copy link
Contributor

@dbbaughe dbbaughe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patience in getting this in @dabr-grapeup and @JavierAbrego :)

qreshi
qreshi approved these changes May 14, 2020
View reviewed changes
Copy link
Contributor

@qreshi qreshi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Thank you for your patience and contributions @dabr-grapeup and @JavierAbrego

@dbbaughe dbbaughe merged commit 4a4d707 into opendistro-for-elasticsearch:master May 14, 2020
@dbbaughe dbbaughe mentioned this pull request May 18, 2020
Closed
@adityaj1107 adityaj1107 mentioned this pull request Jun 3, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@JavierAbrego JavierAbrego JavierAbrego left review comments

@jinsoor-amzn jinsoor-amzn jinsoor-amzn left review comments

@dbbaughe dbbaughe dbbaughe approved these changes

@qreshi qreshi qreshi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@dabr-grapeup @dbbaughe @JavierAbrego @alolita @jinsoor-amzn @qreshi