Add GetSnapshotsIT#testAllFeatures#111786
Conversation
The features of get-snapshots API are all tested in isolation or small combinations, but there's no one test which pins down exactly how they all interact. This commit adds such a test, to verify that any future optimization work preserves the observable behaviour. Relates elastic#95345 Relates elastic#104607
|
Pinging @elastic/es-distributed (Team:Distributed) |
ywangd
left a comment
There was a problem hiding this comment.
LGTM
There is also slm_policy_filter. But I guess it is not something of interest in the context of this work?
| .map(si -> si.snapshotId().getName()) | ||
| .collect(Collectors.toSet()); | ||
| filterByNamePredicate = filterByNamePredicate.and(si -> selectedSnapshots.contains(si.snapshotId().getName())); | ||
| requestedSnapshots = selectedSnapshots.stream().map(n -> n + "*").toArray(String[]::new); |
There was a problem hiding this comment.
Should we randomize the * suffix?
There was a problem hiding this comment.
We can do a bit yeah but not always, see comment added in d5d520e.
| case NAME -> startingSnapshot.snapshotId().getName(); | ||
| case DURATION -> Long.toString(startingSnapshot.endTime() - startingSnapshot.startTime()); | ||
| case INDICES, SHARDS -> Integer.toString(startingSnapshot.indices().size()); | ||
| case FAILED_SHARDS -> "0"; |
There was a problem hiding this comment.
There should be no failed shards. So we effectively do not test it. Should we add a comment?
| nextResponse.getSnapshots().stream().map(SnapshotInfo::snapshotId).toList() | ||
| ); | ||
| assertEquals(nextSize, nextResponse.getSnapshots().size()); | ||
| assertEquals(selectedSnapshots.size(), nextResponse.totalCount()); |
There was a problem hiding this comment.
This is not
| assertEquals(selectedSnapshots.size(), nextResponse.totalCount()); | |
| assertEquals(selectedSnapshots.size() - skippedByFromSortValue, nextResponse.totalCount()); |
?
There was a problem hiding this comment.
Correct. We're not even specifying ?from_sort_value in this request, so we're seeing every matching snapshot in the total. I think we probably should not subtract these skipped items from the original total either, it seems fairly inconsistent to do what we do today, but this behaviour dates back quite some time so we'd need to discuss any changes. It's noted in this comment:
| nextRequestAfter = nextResponse.next(); | ||
| nextExpectedOffset += nextSize; | ||
| remaining -= nextSize; | ||
| } |
There was a problem hiding this comment.
Can we assert remaining == 0 here?
Ah good point, yeah we should cover that in these tests too. I tried a few quick fixes without success, will come back to this in a follow-up. |
Spotted the problem, added coverage in c6de0b4. |
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 `RepositoryData` and must be read from the individual `SnapshotInfo` blobs.
The features of get-snapshots API are all tested in isolation or small combinations, but there's no one test which pins down exactly how they all interact. This commit adds such a test, to verify that any future optimization work preserves the observable behaviour. Relates elastic#95345 Relates elastic#104607
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.
The features of get-snapshots API are all tested in isolation or small combinations, but there's no one test which pins down exactly how they all interact. This commit adds such a test, to verify that any future optimization work preserves the observable behaviour. Relates elastic#95345 Relates elastic#104607
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.
The features of get-snapshots API are all tested in isolation or small
combinations, but there's no one test which pins down exactly how they
all interact. This commit adds such a test, to verify that any future
optimization work preserves the observable behaviour.
Relates #95345
Relates #104607