Test get-snapshots API with missing details#111903
Conversation
Extends the test added in elastic#111786 to check that the API still works correctly even in the BwC case that the details needed are not in the `RepositoryData` and must be read from the individual `SnapshotInfo` blobs.
|
Pinging @elastic/es-distributed (Team:Distributed) |
DiannaHohensee
left a comment
There was a problem hiding this comment.
Taking a first pass to make sure I understand what's going on, and to improve readability / future debugging.
server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java
Outdated
Show resolved
Hide resolved
| assertEquals(0, remaining); | ||
| } | ||
|
|
||
| private static Set<SnapshotId> removeRandomSnapshotDetails(List<String> repositories) { |
There was a problem hiding this comment.
pls comment method with explanation about randomly exercising backwards compatibility, and what backwards compatibility specifically.
| return Set.of(); | ||
| } | ||
| final var masterRepositoriesService = internalCluster().getCurrentMasterNodeInstance(RepositoriesService.class); | ||
| return safeAwait(l0 -> { |
There was a problem hiding this comment.
I have no idea what l0 is abbreviating and thought it was a ten at first -- how about safeAwaitListener?
There was a problem hiding this comment.
:) fair enough, fixed now.
| final var masterRepositoriesService = internalCluster().getCurrentMasterNodeInstance(RepositoriesService.class); | ||
| return safeAwait(l0 -> { | ||
| final Set<SnapshotId> snapshotsWithoutDetails = ConcurrentCollections.newConcurrentSet(); | ||
| try (var listeners = new RefCountingListener(l0.map(v -> Set.copyOf(snapshotsWithoutDetails)))) { |
There was a problem hiding this comment.
| try (var listeners = new RefCountingListener(l0.map(v -> Set.copyOf(snapshotsWithoutDetails)))) { | |
| // Once we finish going through the repositories, complete the safeAwait listener with a copy of snapshotsWithoutDetails. | |
| try (var listeners = new RefCountingListener(l0.map(v -> Set.copyOf(snapshotsWithoutDetails)))) { |
There was a problem hiding this comment.
This took me 10+ minutes to decipher all the utilities and how they work together, I'd really appreciate a comment to boost understanding/retention for the next poor sucker (possibly me again) to come along.
server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java
Outdated
Show resolved
Hide resolved
server/src/internalClusterTest/java/org/elasticsearch/snapshots/GetSnapshotsIT.java
Outdated
Show resolved
Hide resolved
| assertEquals(0, remaining); | ||
| } | ||
|
|
||
| private static Set<SnapshotId> removeRandomSnapshotDetails(List<String> repositories) { |
There was a problem hiding this comment.
Might better to name it removeSnapshotDetailsFromRandomSnapshots, or something. Right now it suggests to me that random details will be removed -- sometimes this field, sometimes that other field. But a comment will clarify 🤷♀️
|
Thanks @DiannaHohensee - this was extracted out of a larger change so yes might not make quite so much sense on its own. Hopefully the changes I just pushed improve things, please take another look. |
* upstream/main: Fail `indexDocs()` on rejection (elastic#111962) Move repo analyzer to its own package (elastic#111963) Add generated evaluators for DateNanos conversion functions (elastic#111961) Clean the last traces from global retention in templates (elastic#111669) Fix known issue docs for elastic#111866 (elastic#111956) x-pack/plugin/otel: introduce x-pack-otel plugin (elastic#111091) Improve reaction to blob store corruptions (elastic#111954) Introduce `StreamingXContentResponse` (elastic#111933) Revert "Add 8.15.0 known issue for memory locking in Windows (elastic#111949)" Test get-snapshots API with missing details (elastic#111903) Add 8.15.0 known issue for memory locking in Windows (elastic#111949) # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java
Extends the test added in elastic#111786 to check that the API still works correctly even in the BwC case that the details needed are not in the `RepositoryData` and must be read from the individual `SnapshotInfo` blobs.
Extends the test added in elastic#111786 to check that the API still works correctly even in the BwC case that the details needed are not in the `RepositoryData` and must be read from the individual `SnapshotInfo` blobs.
Extends the test added in #111786 to check that the API still works
correctly even in the BwC case that the details needed are not in the
RepositoryDataand must be read from the individualSnapshotInfoblobs.