Use InMemoryNoOpCommitDirectory for archives indices only #121210
Conversation
tlrx
added
>non-issue
:Distributed Indexing/Recovery
tlrx
force-pushed
the
2025/01/29/inmemorynoop
branch
from
0e6166d to
e929328
Compare
tlrx
requested review from
fcofdez and
henningandersen
and removed request for
henningandersen
elasticsearchmachine
added
the
Team:Distributed Indexing
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
|
Thanks Francisco! |
henningandersen
left a comment
henningandersen
left a comment
There was a problem hiding this comment.
LateLGTM
(though I have a question).
|
|
||
| @Override | ||
| public void sync(Collection<String> names) { | ||
| throw unsupportedException(); |
There was a problem hiding this comment.
Just so I am sure I understand this correctly, we still call sync and syncMetadata but do not modify anything? I wonder why these are still called?
There was a problem hiding this comment.
syncMetadata is called during peer-recoveries by Store#cleanupAndVerify and if I remember correctly sync is called during snapshots.
Those methods are still called because peer-recovery of searchable snapshots is the same as for regular indices, with the difference that the files on the replica "magically" appears before the recovery starts. We could change that but it was a bit too much work when I tried because the code is intermingled and required to add many "if not searchable snapshot do this" conditions (and also because before we would need to accomodate for searchable snapshots indices before #118606 that creates an additional commit during recovery). It's feasible though, but the code ended up being much less readible.
So I think that relying on SearchableSnapshotDirectory throwing on files change operations would help catching any places where we're changing files whereas after #118606 we should not.
Since I'm not fully confident that we're testing all places I only merged this in 9.1.
…1210) Since elastic#118606 searchable snapshots shards are not expected to write files on disk, with the exception of archives indices mounted as searchable snapshots which require to rewrite the segment infos file in a newer version. Ideally we should be able to remove the usage of the InMemoryNoOpCommitDirectory for non-archives searchable snapshots indices and only rely on SearchableSnapshotDirectory that throws on write operations. Similarly, starting 9.0 searchable snapshots shards do not write files on disk and therefore should be able to use a Directory implementation that forbids writes. Searchable snapshots shards for indices created before 9.0 require a mutable directory for peer-recoveries. In this change, we only allow writes for archives indices and searchable snapshots created before 9.0. Relates ES-10438
… stopped This is caught thanks to elastic#121210: if shard files are verified/checksumed while the node is stopping, an IllegalStateException is throw by CacheService.get() when it attempts to read data from the cache. This exception later caused the verification to fail and then the Lucene index to be marked as corrupted (which nows fails for searchable snapshots shards that are read-only and should not be corrupted at all). This pull request changes ensureLifecycleStarted(), which is called during CacheService.get(), to throw an AlreadyClosedException when the service is stopped (note that ACE extends IllegalStateException, which is convenient here). This ACE will be later specially handlded in the checksumIndex method to not mark the shard as corrupted (see elastic#121210). Closes elastic#121927
… stopped (#122006) This is caught thanks to #121210: if shard files are verified/checksumed while the node is stopping, an IllegalStateException is throw by CacheService.get() when it attempts to read data from the cache. This exception later caused the verification to fail and then the Lucene index to be marked as corrupted (which nows fails for searchable snapshots shards that are read-only and should not be corrupted at all). This pull request changes ensureLifecycleStarted(), which is called during CacheService.get(), to throw an AlreadyClosedException when the service is stopped (note that ACE extends IllegalStateException, which is convenient here). This ACE will be later specially handlded in the checksumIndex method to not mark the shard as corrupted (see #121210). Closes #121927
4 participants
Since #118606 searchable snapshots shards are not expected to write files on disk, with the exception of archives indices mounted as searchable snapshots which require to rewrite the segment infos file in a newer version.
Ideally we should be able to remove the usage of the
InMemoryNoOpCommitDirectoryfor non-archives searchable snapshots indices and only rely onSearchableSnapshotDirectorythat throws on write operations. Similarly, starting 9.0 searchable snapshots shards do not write files on disk and therefore should be able to use a Directory implementation that forbids writes. Searchable snapshots shards for indices created before 9.0 require a mutable directory for peer-recoveries.In this change, we only allow writes for archives indices and searchable snapshots created before 9.0.
Relates ES-10438