Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Changing wait_for_snapshot to check start time rather than finish time #75644

Merged
merged 34 commits into from
Jul 30, 2021
Merged

Changing wait_for_snapshot to check start time rather than finish time #75644

merged 34 commits into from
Jul 30, 2021

Conversation

masseyke
Copy link
Member

The wait_for_snapshot ILM action had been checking whether a snapshot had finished in the given time. However what is more important is that it had started in the given time. Since we weren't capturing the snapshot start time in the state store, most of this PR is about grabbing that information and putting it in the state store so that it is available (without having to do an expensive query).

@masseyke masseyke requested a review from dakrone July 22, 2021 18:25
@dakrone dakrone added the :Data Management/ILM+SLM Index and Snapshot lifecycle management label Jul 22, 2021
dakrone
dakrone requested changes Jul 26, 2021
View reviewed changes
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

I left some comments, I think in a separate change we should also look at renaming the existing timestamp field to indicate that it is the finish time rather than the start time (but since that is a breaking change we should do it in a separate PR)

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/slm/SnapshotInvocationRecord.java Outdated Show resolved Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/slm/SnapshotInvocationRecord.java Outdated Show resolved Hide resolved
x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForSnapshotStepTests.java Outdated Show resolved Hide resolved
x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/slm/SnapshotLifecycleTask.java Outdated Show resolved Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/slm/SnapshotInvocationRecord.java Outdated Show resolved Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/slm/SnapshotInvocationRecord.java Outdated Show resolved Hide resolved
@masseyke
Copy link
Member Author

@elasticmachine run elasticsearch-ci/docs

@masseyke
Copy link
Member Author

@elasticmachine update branch

@masseyke
Copy link
Member Author

@elasticmachine run elasticsearch-ci/part-1

@masseyke masseyke marked this pull request as ready for review July 27, 2021 22:08
@elasticmachine elasticmachine added the Team:Data Management Meta label for data/management team label Jul 27, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features (Team:Core/Features)

dakrone
dakrone requested changes Jul 28, 2021
View reviewed changes
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

Thanks Keith, I left some comments, nothing too major though.

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ilm/WaitForSnapshotStep.java Outdated Show resolved Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/slm/SnapshotInvocationRecord.java Outdated Show resolved Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/slm/SnapshotInvocationRecord.java Show resolved Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/slm/SnapshotInvocationRecord.java Outdated Show resolved Hide resolved
x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/slm/SnapshotInvocationRecord.java Outdated Show resolved Hide resolved
...lti-node/src/javaRestTest/java/org/elasticsearch/xpack/ilm/TimeSeriesLifecycleActionsIT.java Show resolved Hide resolved
x-pack/plugin/ilm/src/main/java/org/elasticsearch/xpack/slm/SnapshotLifecycleTask.java Outdated Show resolved Hide resolved
dakrone
dakrone approved these changes Jul 28, 2021
View reviewed changes
Copy link
Member

@dakrone dakrone left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for iterating on it Keith!

@gwbrown gwbrown mentioned this pull request Jul 28, 2021
Closed
@masseyke
Copy link
Member Author

@elasticmachine run elasticsearch-ci/part-2

@masseyke
Copy link
Member Author

@elasticmachine update branch

@masseyke
Copy link
Member Author

@elasticmachine run elasticsearch-ci/part-2

@masseyke
Copy link
Member Author

@elasticmachine run elasticsearch-ci/part-2

@masseyke masseyke merged commit 3d6c428 into elastic:master Jul 30, 2021
@masseyke masseyke deleted the fix/73357 branch July 30, 2021 18:58
@masseyke masseyke linked an issue Jul 30, 2021 that may be closed by this pull request
ILM uses end-time of SLM snapshot when waiting for snapshot #73357
Closed
@masseyke masseyke mentioned this pull request Jul 30, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dakrone dakrone dakrone approved these changes

Assignees
No one assigned
Labels
>bug :Data Management/ILM+SLM Index and Snapshot lifecycle management Team:Data Management Meta label for data/management team v7.15.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ILM uses end-time of SLM snapshot when waiting for snapshot
6 participants
@masseyke @elasticmachine @dakrone @joegallo @mark-vieira @elasticsearchmachine