Compute SLM retention from RepositoryData#100092
Compute SLM retention from RepositoryData#100092elasticsearchmachine merged 11 commits intoelastic:mainfrom
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
f521bc3 to
f060f60
Compare
This comment was marked as resolved.
This comment was marked as resolved.
There is no need to obtain `SnapshotInfo` for all snapshots in order to compute SLM retention. With this commit we move to computing it directly from the `RepositoryData` in most circumstances, and in rare situations where we must still retrieve `SnapshotInfo` blobs we make sure not to hold many in memory at once. Closes elastic#99953
f060f60 to
c8ea2d3
Compare
|
Hi @DaveCTurner, I've created a changelog YAML for you. |
|
Pinging @elastic/es-data-management (Team:Data Management) |
x-pack/plugin/slm/src/main/java/org/elasticsearch/xpack/slm/SLMGetExpiredSnapshotsAction.java
Show resolved
Hide resolved
x-pack/plugin/slm/src/main/java/org/elasticsearch/xpack/slm/SLMGetExpiredSnapshotsAction.java
Outdated
Show resolved
Hide resolved
andreidan
left a comment
There was a problem hiding this comment.
Thanks for working on this David. This is looking really good.
I left a few minor suggestions
| } | ||
| } | ||
|
|
||
| private static List<Tuple<SnapshotId, String>> getSnapshotsToDelete( |
There was a problem hiding this comment.
Thanks for all the details in this function ❤️ It's a real pleasure to read.
Would we benefit from a bit more targeted testing for this function if it was package-visible and target every if/else branch with unit tests? (similar to the previous SnapshotRetentionTask#snapshotEligibleForDeletion)
There was a problem hiding this comment.
I'm not sure this adds much (the existing test already covers all the branches) but I added some more focussed tests in eef1e63.
| } | ||
| } | ||
|
|
||
| private static void getSnapshotDetailsByPolicy( |
There was a problem hiding this comment.
Same as below, perhaps a personal preference, but should we add a bit more targeted unit testing to this function?
| } | ||
|
|
||
| private static class ResultsBuilder { | ||
| private final Map<String, List<Tuple<SnapshotId, String>>> resultsByRepository = ConcurrentCollections.newConcurrentMap(); |
There was a problem hiding this comment.
nit: we could use the ConcurrentMap interface directly here to indicate thread-safety and atomicity guarantees are needed
There was a problem hiding this comment.
Eh we could but that makes the declaration long enough that it splits over two lines. Moreover we never mention its type again in its 11-line scope.
| import java.util.function.BiFunction; | ||
| import java.util.stream.Stream; | ||
|
|
||
| public class SLMGetExpiredSnapshotsAction extends ActionType<SLMGetExpiredSnapshotsAction.Response> { |
There was a problem hiding this comment.
As this is a public class, should we add a small javadoc?
There is no need to obtain
SnapshotInfofor all snapshots in order tocompute SLM retention. With this commit we move to computing it directly
from the
RepositoryDatain most circumstances, and in rare situationswhere we must still retrieve
SnapshotInfoblobs we make sure not tohold many in memory at once.
Closes #99953