Allow archive and searchable snapshots indices in N-2 version#118941
Allow archive and searchable snapshots indices in N-2 version#118941tlrx merged 12 commits intoelastic:mainfrom
Conversation
Relates ES-10274
tlrx
added
>enhancement
:Distributed/Recovery
|
Hi @tlrx, I've created a changelog YAML for you. |
elasticsearchmachine
added
the
Team:Distributed Indexing (obsolete)
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
server/src/main/java/org/elasticsearch/common/lucene/Lucene.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/engine/ReadOnlyEngine.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/engine/ReadOnlyEngine.java
Outdated
Show resolved
Hide resolved
| return readSegmentsInfo(null, directory()); | ||
| } catch (CorruptIndexException | IndexFormatTooOldException | IndexFormatTooNewException ex) { | ||
| } catch (IndexFormatTooOldException e) { | ||
| // Temporary workaround |
There was a problem hiding this comment.
could you expand on why we need this workaround? I could see how we need special logic for the case where we read from a specific index commit, but given the argument here is null, I would suggest to just change Store#readSegmentsInfo to provide the major argument at all times and then we no longer need this special handling? We do need to handle the case for reading the from a specific index comment pending 10.1 though because the method is not yet public in Lucene
There was a problem hiding this comment.
Same for all the special casing done in this class, I think that we should centralize the segment infos parsing in a single place, and always provide the min supported major argument, I don't see why not. Not doing so complicates things for no reason?
There was a problem hiding this comment.
Yeah sorry, I've been a bit nuclear here to have the tests passed.
I reworked this in the latest commit so that all methods finally end using one of the Lucene#readSegmentInfos() methods. I implemented a workaround there for reading a specific commit in N-2 version, let me know what you think.
There was a problem hiding this comment.
I messed up exception handling in one of my latest change so be sure to review 612cde2
henningandersen
left a comment
henningandersen
left a comment
There was a problem hiding this comment.
Looks great. Not approving due to:
more changes to allow reading Lucene commit files in N-2 version but those can be disregarded for now
server/src/main/java/org/elasticsearch/cluster/metadata/IndexMetadataVerifier.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/lucene/Lucene.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/index/engine/ReadOnlyEngine.java
Outdated
Show resolved
Hide resolved
| private static SegmentInfos readSegmentInfos(String segmentsFileName, Directory directory) throws IOException { | ||
| return SegmentInfos.readCommit(directory, segmentsFileName); | ||
| // TODO Use readCommit(Directory directory, String segmentFileName, int minSupportedMajorVersion) once Lucene 10.1 is available | ||
| // and remove the try-catch block for IndexFormatTooOldException |
There was a problem hiding this comment.
could you add an assert based on the lucene version here just to make 100% sure?
javanna
left a comment
javanna
left a comment
There was a problem hiding this comment.
left a couple of nits, LGTM though!
…118923) This is the backport of #118941 for 8.18. This change relaxes the index compatibility version checks to allow archive and searchable snapshot indices in version N-2 to exist on a 9.x cluster. It uses the min. read-only index compatible version added in #118884 to accept join requests from 9.x nodes that can read indices created in version 7.x (N-2) as long as they have a write block and are either archive or searchable snapshot indices. Relates ES-10274
Fixes #1200 Updates the [Index compatibility](https://www.elastic.co/docs/deploy-manage/tools/snapshot-and-restore#index-compatibility) table in the **Snapshot and restore** section of the deployment docs. [Preview available here](https://docs-v3-preview.elastic.dev/elastic/docs-content/pull/1617/deploy-manage/tools/snapshot-and-restore#index-compatibility) These changes are [confirmed by eng](#1200 (comment)) and also [PR#118941](elastic/elasticsearch#118941) provides additional context on the compatibility.
4 participants
This change (along with the required change #118923 for 8.18) relaxes the index compatibility version checks to allow archive and searchable snapshot indices in version N-2 to exist on a 9.x cluster.
The change in the compatibility tests and
NodeJoinExecutorandIndexMetadataVerifierare ready for review.Relates ES-10274