Skip translog creation and Lucene commits when recovering searchable snapshot shards#118606
Merged
tlrx merged 3 commits intoelastic:mainfrom Dec 16, 2024
Conversation
d029b12 to
a19e312
Compare
Collaborator
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
tlrx
commented
Dec 13, 2024
| */ | ||
| public boolean hasTranslog() { | ||
| // Expect no translog files to exist for searchable snapshots | ||
| return false == indexSettings.getIndexMetadata().isSearchableSnapshot(); |
Member
Author
There was a problem hiding this comment.
I limited the scope to searchable snapshot shards (including closed searchable snapshot shards).
tlrx
commented
Dec 13, 2024
| try { | ||
| final var translogLocation = indexShard.shardPath().resolveTranslog(); | ||
| if (indexShard.hasTranslog() == false) { | ||
| Translog.deleteAll(translogLocation); |
Member
Author
There was a problem hiding this comment.
We may want to relax this for regular read-only indices later.
a19e312 to
5e0224b
Compare
henningandersen
approved these changes
Dec 13, 2024
Contributor
henningandersen
left a comment
There was a problem hiding this comment.
LGTM.
Left a few minor comments, but no need for an additional round.
server/src/main/java/org/elasticsearch/index/translog/TranslogConfig.java
Outdated
Show resolved
Hide resolved
.../old-lucene-versions/src/main/java/org/elasticsearch/xpack/lucene/bwc/OldLuceneVersions.java
Outdated
Show resolved
Hide resolved
| assertThat(shardRouting.toString(), unwrappedDir, instanceOf(ByteBuffersDirectory.class)); | ||
|
|
||
| final ByteBuffersDirectory inMemoryDir = (ByteBuffersDirectory) unwrappedDir; | ||
| assertThat(inMemoryDir.listAll(), arrayWithSize(1)); |
Contributor
There was a problem hiding this comment.
Could we leave this in place but verify there are no files modified - at least until we possibly remove the in-memory-no-op-commit-directory?
Member
Author
There was a problem hiding this comment.
Makes sense yes, that's a good reason to keep this.
5e0224b to
7af707c
Compare
Member
Author
|
Thanks Henning & Luca! |
tlrx
added a commit
that referenced
this pull request
Feb 4, 2025
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 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
fzowl
pushed a commit
to voyage-ai/elasticsearch
that referenced
this pull request
Feb 4, 2025
…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
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.
In order to leverage Lucene N-2 version support for searchable snapshots, we'd like to avoid executing Lucene commits during searchable snapshots shards recovery. This is because Lucene commits require to open an
IndexWriter, something that Lucene does not support for N-2 versions.Today when searchable snapshot shards are recovering they create a translog on disk as well as a Lucene commit:
We can skip the creation of the translog because searchable snapshot shard do not need one, and it's only use to store the local checkpoint locally to be read later during recovery. If we don't have a translog then we don't need to associate it with the Lucene index, so we can skip the commit too.
This change introduce an
hasTranslogmethod that is used to know when it is safe to NOT create a translog, in which case the global checkpoint is read from the last Lucene commit during primary shard recovery from snapshot, peer-recovery and recovery from existing store.In case an existing translog exist on disk, it will be cleaned up.
They are also few discoveries around some assertions introduced with snapshot based recoveries, as well as a cached estimation of the size of directories that was refreshed due to Lucene commit but now requires to be "marked as stale".