Rework get-snapshots predicates#143161
Merged
DaveCTurner merged 6 commits intoelastic:mainfrom Feb 27, 2026
Merged
Conversation
The inner `SnapshotPredicates` class actually only deals with the `?from_sort_value` filter, and the related `?after` filter was implemented within `SnapshotSortKey` itself. As a step towards adding a pre-flight `?after` check this commit refactors the `?after` functionality into a new `AfterPredicates` class, and for the sake of consistency moves the `?from_sort_value` functionality to an adjacent top-level `FromSortValuePredicates` class.
DaveCTurner
added
:Distributed/Snapshot/Restore
Collaborator
|
Pinging @elastic/es-distributed (Team:Distributed) |
DaveCTurner
requested review from
inespot and
ywangd
and removed request for
inespot
inespot
approved these changes
Feb 26, 2026
Contributor
inespot
left a comment
inespot
left a comment
There was a problem hiding this comment.
Nice refactor, the code is so much more readable now 🎉 A couple small comments/questions
| private final SnapshotNamePredicate snapshotNamePredicate; | ||
| private final SnapshotPredicates fromSortValuePredicates; | ||
| private final FromSortValuePredicates fromSortValuePredicates; | ||
| private final Predicate<String> slmPolicyPredicate; |
Contributor
There was a problem hiding this comment.
Should this also be declared SlmPolicyPredicate (might have to tweak the SlmPolicyPredicate a little to resemble the AfterPredicates) and we can move it out into its own class, to be consistent and make TransportGetSnapshotsAction more succinct? Maybe as a FLUP PR
Member
Author
There was a problem hiding this comment.
I'm inclined to keep it like this for now - it doesn't depend on the sortBy so it's much simpler than the ?from_sort_value and ?after parameters.
| * Predicate for the {@code ?after} filter of the get-snapshots action. The {@link #test(SnapshotInfo)} predicate is applied to | ||
| * {@link SnapshotInfo} instances to filter out those that sort before the cursor value (i.e. were returned on earlier pages of results). | ||
| */ | ||
| public final class AfterPredicates { |
Contributor
There was a problem hiding this comment.
Should we add a quick unit test suite for this?
| * Predicate for the {@code ?after} filter of the get-snapshots action. The {@link #test(SnapshotInfo)} predicate is applied to | ||
| * {@link SnapshotInfo} instances to filter out those that sort before the cursor value (i.e. were returned on earlier pages of results). | ||
| */ | ||
| public final class AfterPredicates { |
Contributor
There was a problem hiding this comment.
Can AfterPredicates be package-private?
PeteGillinElastic
pushed a commit
to PeteGillinElastic/elasticsearch
that referenced
this pull request
Feb 27, 2026
The inner `SnapshotPredicates` class actually only deals with the `?from_sort_value` filter, and the related `?after` filter was implemented within `SnapshotSortKey` itself. As a step towards adding a pre-flight `?after` check this commit refactors the `?after` functionality into a new `AfterPredicates` class, and for the sake of consistency moves the `?from_sort_value` functionality to an adjacent top-level `FromSortValuePredicates` class.
szybia
added a commit
to szybia/elasticsearch
that referenced
this pull request
Feb 27, 2026
…cations * upstream/main: (35 commits) Create ARM bulk sqrI8 implementation (elastic#142461) Rework get-snapshots predicates (elastic#143161) Refactor downsampling fetchers and producers (elastic#140357) ESQL: Unmute test and add extra logging to generative test validation (elastic#143168) Fix metadata fields being nullified/loaded by unmapped_fields setting (elastic#143155) Determine remote cluster version (elastic#142494) Populate failure message for aborted clones (elastic#143206) Allow kibana_system role to read and manage logs streams (elastic#143053) Mute org.elasticsearch.xpack.esql.CsvIT test {csv-spec:eval.DocsLength} elastic#143224 Mute org.elasticsearch.xpack.esql.CsvIT test {csv-spec:eval.DocsByteLength} elastic#143223 Mute org.elasticsearch.xpack.esql.CsvIT test {csv-spec:docs.DocsBitLength} elastic#143222 Fix FloatVectorScorerSupplier bulkScore bug (elastic#143211) ESQL: Add data node execution for external sources (elastic#143209) [ESQL] Cleanup commands docs (elastic#143058) [ML]Fix latest transforms disregarding updates when sort and sync fields are non-monotonic (elastic#142856) Mute org.elasticsearch.index.mapper.IpFieldMapperTests testSyntheticSourceInObject elastic#143212 Tests: Fix StoreDirectoryMetricsIT (elastic#143084) ESQL: Add distribution strategy for external sources (elastic#143194) CSV IT spec (elastic#142585) Fix VectorScorerOSQBenchmark.score to read corrections properly (elastic#143137) ...
tballison
pushed a commit
to tballison/elasticsearch
that referenced
this pull request
Mar 3, 2026
The inner `SnapshotPredicates` class actually only deals with the `?from_sort_value` filter, and the related `?after` filter was implemented within `SnapshotSortKey` itself. As a step towards adding a pre-flight `?after` check this commit refactors the `?after` functionality into a new `AfterPredicates` class, and for the sake of consistency moves the `?from_sort_value` functionality to an adjacent top-level `FromSortValuePredicates` class.
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
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.
The inner
SnapshotPredicatesclass actually only deals with the?from_sort_valuefilter, and the related?afterfilter wasimplemented within
SnapshotSortKeyitself. As a step towards adding apre-flight
?aftercheck this commit refactors the?afterfunctionality into a new
AfterPredicatesclass, and for the sake ofconsistency moves the
?from_sort_valuefunctionality to an adjacenttop-level
FromSortValuePredicatesclass.