-
Notifications
You must be signed in to change notification settings - Fork 24.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ensure FrozenEngine always applies engine-level wrapper #76100
Conversation
Pinging @elastic/es-distributed (Team:Distributed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a nit in tests, LGTM.
...ava/org/elasticsearch/xpack/searchablesnapshots/AbstractSearchableSnapshotsRestTestCase.java
Outdated
Show resolved
Hide resolved
Thanks @dnhatn |
FrozenEngine does not always apply the engine-level DirectoryReader wrapper. Only affects combination of source-only snapshot repositories and shared_cache searchable snapshots. While it is technically a bug, it is such an exotic combination of things that it might not be even worth calling out. The fix is mostly here to ensure that future wrappers won't suffer from the same fate.
…)" This reverts commit 90c3de8.
…)" This reverts commit 6f31abc.
I've reverted this PR as it uncovered some interesting existing bugs with source-only snapshots on CI (that have nothing to do with this PR, but the PR better exercises the existing functionality). The test exercised with this PR was using multiple shards (in contrast to SourceOnlySnapshotIT, which just uses a single shard), which led to the discovery that sorting by _seq_no when searching on a source-only snapshot (something explicitly tested) breaks in the can-match phase (which only comes into play when there are multiple shards) as the wrapper does not properly implement min-max retrieval. This was broken when #49092 was added, but the source-only snapshot tests were not general enough to catch it.
|
FrozenEngine does not always apply the engine-level DirectoryReader wrapper. Only affects combination of source-only snapshot repositories and shared_cache searchable snapshots. While it is technically a bug, it is such an exotic combination of things that it might not be even worth calling out. The fix is mostly here to ensure that future wrappers won't suffer from the same fate.
The test now exercises loading
_seq_no
(which is added by wrapper) when runningAbstractSearchableSnapshotsRestTestCase.testSourceOnlyRepository
.