Move SLM eligibility check #100044
Merged
Move SLM eligibility check #100044
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A small refactoring to make elastic#99953 a little simpler: combine the logic for retrieving the snapshot info and filtering out the ineligible ones into a single function so we can replace it with a call to a dedicated client action in a followup.
DaveCTurner
added
>refactoring
:Data Management/ILM+SLM
elasticsearchmachine
added
the
Team:Data Management
Collaborator
|
Pinging @elastic/es-data-management (Team:Data Management) |
andreidan
approved these changes
Sep 29, 2023
Contributor
andreidan
left a comment
andreidan
left a comment
There was a problem hiding this comment.
LGTM, thanks for doing this refactoring David
Comment on lines
+300
to
+301
| // SnapshotInfo instances can be quite large in case they contain e.g. a large collection of | ||
| // exceptions so we extract the only two things (id + policy id) here so they can be GCed |
Contributor
There was a problem hiding this comment.
++ very nice, thanks explaining the why behind it
Should we consider adding support for this kind of filtering at the API level? (perhaps a fields query parameter that specifies only the fields we want in the response?)
Contributor
Author
There was a problem hiding this comment.
This comment was moved as-is from the previous code :)
We're going to fix this properly in follow-ups, but there's basically no reasonable way to use the get-snapshots API here so we'll do something else.
DaveCTurner
deleted the
2023/09/29/slm-snapshot-move-eligibility-check
branch
piergm
pushed a commit
to piergm/elasticsearch
that referenced
this pull request
Oct 2, 2023
A small refactoring to make elastic#99953 a little simpler: combine the logic for retrieving the snapshot info and filtering out the ineligible ones into a single function so we can replace it with a call to a dedicated client action in a followup.
jakelandis
pushed a commit
to jakelandis/elasticsearch
that referenced
this pull request
Oct 2, 2023
A small refactoring to make elastic#99953 a little simpler: combine the logic for retrieving the snapshot info and filtering out the ineligible ones into a single function so we can replace it with a call to a dedicated client action in a followup.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A small refactoring to make #99953 a little simpler: combine the logic
for retrieving the snapshot info and filtering out the ineligible ones
into a single function so we can replace it with a call to a dedicated
client action in a followup.