Add support for peer recoveries using snapshots after primary failovers by fcofdez · Pull Request #77420 · elastic/elasticsearch

fcofdez · 2021-09-08T11:43:37Z

This commit adds support for peer recoveries using snapshots after
a primary failover if the snapshot shares the same logical contents
but the physical files are different. It uses the seq no information
stored in the snapshot to compare against the current shard source
node seq nos and decide whether or not it can use the snapshot to
recover the shard. Since the underlying index files are different
to the source index files, error handling is different than when
the files are shared. In this case, if there's an error while
snapshots files are recovered, we have to cancel the on-going
downloads, wait until all in-flight operations complete, remove
the recovered files and start from scratch using a fallback
recovery plan that uses the files from the source node.

Relates #73496

elasticmachine · 2021-09-08T11:43:39Z

Pinging @elastic/es-distributed (Team:Distributed)

This commit adds support for peer recoveries using snapshots after a primary failover if the snapshot shares the same logical contents but the phyisical files are different. It uses the seq no information stored in the snapshot to compare against the current shard source node seq nos and decide whether or not it can use the snapshot to recover the shard. Since the underlying index files are different to the source index files, error handling is different than when the files are shared. In this case, if there's an error while snapshots files are recovered, we have to cancel the on-going downloads, wait until all in-flight operations complete, remove the recovered files and start from scratch using a fallback recovery plan that uses the files from the source node. Relates elastic#73496

fcofdez · 2021-09-08T13:57:23Z

...r/src/main/java/org/elasticsearch/indices/recovery/plan/SnapshotsRecoveryPlannerService.java

+            Store.RecoveryDiff recoveryDiff = latestSnapshot.getMetadataSnapshot().recoveryDiff(sourceMetadata);
+
+            // Primary failed over after the snapshot was taken
+            if (recoveryDiff.different.isEmpty() == false || recoveryDiff.missing.isEmpty() == false) {


I wonder if we should do some version checks here, since we're not relying on file contents being equal as we do in the regular path.

Good point. AFAICS we do not have that info currently. I wonder if we should add Version.CURRENT to commit user data when committing. But it does not get us the full way, we will need a way to represent that in the snapshot too. Let us touch base on that tomorrow.

henningandersen

I reviewed the production part of the PR and have a few initial comments.

henningandersen · 2021-09-08T15:15:53Z

server/src/main/java/org/elasticsearch/indices/recovery/RecoverySourceHandler.java

+            recoverSnapshotFiles(shardRecoveryPlan, recoverSnapshotFilesStep.delegateResponse((delegate, e) -> {
+                if (shardRecoveryPlan.canRecoverSnapshotFilesFromSourceNode() == false &&
+                    e instanceof CancellableThreads.ExecutionCancelledException == false) {
+                    recoveryTarget.deleteRecoveredFiles(new ActionListener<>() {


I wonder if we need this explicit call to the target? I think we could just clean this up when the target receives the first chunk of the "normal" file based recovery? The boolean flag shardRecoveryPlan.canRecoverSnapshotFilesFromSourceNode() could be sent in the receiveFileInfo call.

Addressed in e48a0cb

henningandersen · 2021-09-08T15:26:02Z

...r/src/main/java/org/elasticsearch/indices/recovery/plan/SnapshotsRecoveryPlannerService.java


        ShardSnapshot latestSnapshot = latestSnapshotOpt.get();

+        if (latestSnapshot.isLogicallyEquivalent(shardStateIdentifier)) {


I wonder if we should only allow this case when sourceTargetDiff.identical.isEmpty()? The primary case we are after here is relocations, which will be to a node that does not have the shard at all.

It is unlikely to make a big difference due to retention leases, which ensure that for normal restart cases we do not do any file based recovery at all. But if we find identical files between source and target, it is a trade-off whether it is worth recovering from snapshot. In the interest of this change being a "purely positive" change, I would prefer that we skip recovery from snapshot in that case. Let me know your thoughts on this.

That makes sense and I think that it simplifies reasoning about this edge case too. I'll change it 👍

Addressed in 9f07fde

henningandersen · 2021-09-08T15:57:03Z

...r/src/main/java/org/elasticsearch/indices/recovery/plan/SnapshotsRecoveryPlannerService.java

+            Store.RecoveryDiff recoveryDiff = latestSnapshot.getMetadataSnapshot().recoveryDiff(sourceMetadata);
+
+            // Primary failed over after the snapshot was taken
+            if (recoveryDiff.different.isEmpty() == false || recoveryDiff.missing.isEmpty() == false) {


Good point. AFAICS we do not have that info currently. I wonder if we should add Version.CURRENT to commit user data when committing. But it does not get us the full way, we will need a way to represent that in the snapshot too. Let us touch base on that tomorrow.

henningandersen · 2021-09-08T20:11:38Z

server/src/main/java/org/elasticsearch/indices/recovery/RecoverySourceHandler.java

+                    recoveryTarget.deleteRecoveredFiles(new ActionListener<>() {
+                        @Override
+                        public void onResponse(Void unused) {
+                            recoverFilesFromSourceAndSnapshot(shardRecoveryPlan.getFallbackPlan(), store, stopWatch, listener);


It is a bit worrying that we abandon the rest of the original recoverFilesFromSourceAndSnapshot, i.e., the steps that have registered for whenComplete are never completed. I think it works now, but if we ever expect those to be fired to free some resource it could become a subtle leak.

I think I would prefer to just send the file infos again and then continue on the original steps, seems less confusing. Let me know what you think.

Addressed in e48a0cb

henningandersen

Thanks Francisco, this direction looks good. I left a number of smaller comments for the production code, did not get through tests yet.

server/src/main/java/org/elasticsearch/index/engine/Engine.java

henningandersen · 2021-09-13T07:47:10Z

server/src/main/java/org/elasticsearch/indices/recovery/plan/ShardSnapshot.java

+    }
+
+    @Nullable
+    public Version getVersion() {


Perhaps rename this to getCommitVersion() to be sure not to confuse it with some of the other versions involved in snapshots.

We could also move this to Store.MetadataSnapshot, looks like we might as well construct that in the constructor with the lucene commit data. It seems like a slightly more logical place for this method to live? We can also look at that in a follow-up if you prefer.

henningandersen · 2021-09-13T07:48:28Z

...r/src/main/java/org/elasticsearch/indices/recovery/plan/SnapshotsRecoveryPlannerService.java

+        // Primary failed over after the snapshot was taken
+        if (latestSnapshot.isLogicallyEquivalent(shardStateIdentifier) &&
+            latestSnapshot.hasDifferentPhysicalFiles(sourceMetadata) &&
+            snapshotVersion != null && snapshotVersion.onOrBefore(Version.CURRENT) &&


Let us add a comment here that we check against source version and why it is OK (the allocation decider).

Also, I think we can do following instead:

Suggested change

snapshotVersion != null && snapshotVersion.onOrBefore(Version.CURRENT) &&

(snapshotVersion == null || snapshotVersion.onOrBefore(Version.CURRENT)) &&

and add a comment that no snapshotCommitVersion means that it was taken before 7.16? That would make this new path work from 7.16 with snapshots taken in 7.15-.

henningandersen · 2021-09-13T07:58:18Z

...r/src/main/java/org/elasticsearch/indices/recovery/plan/SnapshotsRecoveryPlannerService.java

+        // Primary failed over after the snapshot was taken
+        if (latestSnapshot.isLogicallyEquivalent(shardStateIdentifier) &&
+            latestSnapshot.hasDifferentPhysicalFiles(sourceMetadata) &&
+            snapshotVersion != null && snapshotVersion.onOrBefore(Version.CURRENT) &&


I wonder if we should also assert on the lucene version also being ok? For the snapshotVersion == null case we could consider checking that the lucene version is before the expected version.

henningandersen · 2021-09-13T09:32:01Z

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

                    commitData.put(FORCE_MERGE_UUID_KEY, currentForceMergeUUID);
                }
                commitData.put(Engine.MIN_RETAINED_SEQNO, Long.toString(softDeletesPolicy.getMinRetainedSeqNo()));
+                commitData.put(ES_VERSION, Version.CURRENT.toString());


I think we need to add this in a few other places like Store.trimUnsafeCommits and StoreRecovery.addIndices?

Can we perhaps add an assertion about the previous version being before or on current version?

And maybe even add an assertion about this being filled in if lucene version is new enough? We can at least keep that in master though we may have to remove it in 7.x depending on whether a lucene version upgrade happens for the release where this lands.

I did not see the two assertions added, perhaps there were issues doing so? If so, please let me know.

Sorry I missed that comment.

Can we perhaps add an assertion about the previous version being before or on current version?

Do you mean asserting that the commit user data that doesn't contain the ES_VERSION value is in a version < 7.16? @henningandersen

My intention with that specific comment was that if there is a version, it must be onOrBefore current version. I.e., the version can only go forward never backwards. This will be verified a little bit once we do some rolling upgrade tests between 7.x and 8.0 (i.e., after merge to both). And obviously as we add releases this check will be more valuable. Just want it to be expressed in an assertion that it is monotonically increasing.

I think we also need to update the version in RemoveCorruptedShardCommand.addNewHistoryCommit and Store.associateIndexWithNewTranslog. About the latter, I wonder if we should just let Store.updateCommitData set the ES version, seems fair to centralize that and would ensure new users of that also update the version.

server/src/main/java/org/elasticsearch/indices/recovery/RecoveryTarget.java

henningandersen · 2021-09-13T09:54:08Z

server/src/main/java/org/elasticsearch/indices/recovery/plan/ShardSnapshot.java

        this.shardSnapshotInfo = shardSnapshotInfo;
        this.snapshotFiles = snapshotFiles.stream()
            .collect(Collectors.toMap(snapshotFile -> snapshotFile.metadata().name(), Function.identity()));
        this.metadataSnapshot = convertToMetadataSnapshot(snapshotFiles);


We might as well pass in the luceneCommitUserData to convertToMetadataSnapshot now that we have it? Makes the metadata snapshot more complete.

henningandersen · 2021-09-13T09:58:42Z

server/src/main/java/org/elasticsearch/indices/recovery/plan/ShardSnapshotsService.java

+                .collect(Collectors.toMap(StoreFileMetadata::name, Function.identity()));
+
+            InMemoryDirectory directory = new InMemoryDirectory(snapshotFiles);
+            SegmentInfos segmentCommitInfos = Lucene.readSegmentInfos(directory);


In the (albeit edge) case that this runs on an older version than the replica and the snapshot is taken on the newer version, this no longer allows file-comparison based recover from snapshot. I think that is OK, but also that it deserves a comment here on this and why it is unlikely to be an issue.

Maybe we can extract that logic into a method and return an empty map if we fail to load the commit info?

I think we would need to be able to distinguish the case where we can read the lucene commit but there is no version from the case where we cannot read the lucene commit, at least if we follow my proposal here

henningandersen · 2021-09-13T10:01:29Z

server/src/main/java/org/elasticsearch/indices/recovery/plan/ShardSnapshotsService.java

        return clusterService.state().nodes().getMinNodeVersion().onOrAfter(SNAPSHOT_RECOVERIES_SUPPORTED_VERSION);
    }
+
+    private static final class InMemoryDirectory extends BaseDirectory {


Maybe rename to StoreFileMetadataDirectory?

henningandersen · 2021-09-13T10:04:28Z

server/src/main/java/org/elasticsearch/indices/recovery/plan/ShardSnapshotsService.java

+
+        @Override
+        public void close() {
+            // no-op


I wonder if this should set isOpen=false? But I also wonder if we should not just extend Directory rather than BaseDirectory, the locking in BaseDirectory seems unnecessary here? None of this is really important though.

fcofdez · 2021-09-14T12:24:25Z

I think I've covered all the review comments and this should be ready for another review round.

henningandersen

I added a few more comments, still need to run through the tests, but will not get to that today.

henningandersen · 2021-09-20T11:23:00Z

server/src/main/java/org/elasticsearch/index/engine/Engine.java

    public static final String MIN_RETAINED_SEQNO = "min_retained_seq_no";
    public static final String MAX_UNSAFE_AUTO_ID_TIMESTAMP_COMMIT_ID = "max_unsafe_auto_id_timestamp";
+    // Field name that stores the Elasticsearch version in Lucene commit user data, representing
+    // the version that was used to write those Lucene segments


Suggested change

// the version that was used to write those Lucene segments

// the version that was used to write the commit (and thus a max version for the underlying segments).

henningandersen · 2021-09-20T11:28:03Z

server/src/main/java/org/elasticsearch/index/store/Store.java

-                    writer.setLiveCommitData(startingIndexCommit.getUserData().entrySet());
+                    final Map<String, String> userData = startingIndexCommit.getUserData();
+                    writer.setLiveCommitData(() -> {
+                        if (userData.containsKey(ES_VERSION)) {


I think this is questionable. If we trim the commit, we store it by the new ES version, which could be on a newer lucene version as well. With this, I think we risk seeing an ES version for an older release but in a commit that is a new lucene version. I think we should avoid that and simply always put in the current ES version. I also think trying to optimize this by not rebuilding the hashmap here is not worth it, this is not performance sensitive code.

henningandersen · 2021-09-20T11:29:09Z

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

                    commitData.put(FORCE_MERGE_UUID_KEY, currentForceMergeUUID);
                }
                commitData.put(Engine.MIN_RETAINED_SEQNO, Long.toString(softDeletesPolicy.getMinRetainedSeqNo()));
+                commitData.put(ES_VERSION, Version.CURRENT.toString());


I did not see the two assertions added, perhaps there were issues doing so? If so, please let me know.

henningandersen · 2021-09-20T11:31:12Z

server/src/main/java/org/elasticsearch/indices/recovery/RecoverySourceHandler.java

                            fallbackPlan.getTranslogOps(),
-                            true,
-                            recoverSnapshotFilesStep.map(r -> Tuple.tuple(fallbackPlan, Collections.emptyList()))
+                                recoverSnapshotFilesStep.map(r -> Tuple.tuple(fallbackPlan, Collections.emptyList()))


Is there an indentation issue here?

henningandersen · 2021-09-20T11:41:16Z

...r/src/main/java/org/elasticsearch/indices/recovery/plan/SnapshotsRecoveryPlannerService.java

+        // if the snapshotVersion == null that means that the snapshot was taken in a version <= 7.15,
+        // therefore we can safely use that snapshot.
+        if (commitVersion == null) {
+            return Version.CURRENT.luceneVersion.onOrAfter(snapshot.getCommitLuceneVersion());


Can we add an assert about this too?

Suggested change

return Version.CURRENT.luceneVersion.onOrAfter(snapshot.getCommitLuceneVersion());

assert Version.V_7_16_0.luceneVersion.onOrAfter(snapshot.getCommitLuceneVersion());

return Version.CURRENT.luceneVersion.onOrAfter(snapshot.getCommitLuceneVersion());

henningandersen · 2021-09-20T11:44:02Z

...r/src/main/java/org/elasticsearch/indices/recovery/plan/SnapshotsRecoveryPlannerService.java

        );
    }

+    private boolean isSnapshotVersionCompatible(ShardSnapshot snapshot) {


Can we add a comment that this runs on the primary, but using it's version is fine, since NodeVersionAllocationDecider ensures we only recover to a node that has newer or same version.

henningandersen · 2021-09-20T11:47:53Z

server/src/main/java/org/elasticsearch/indices/recovery/plan/ShardSnapshotsService.java

+            // (i.e. the snapshot was taken in a node with version > than this node version)
+            // reading the segment commit information could likely fail and we won't be able
+            // to recover from a snapshot. This should be a rare edge-case since for most cases
+            // the allocation deciders won't allow allocating replicas in nodes with older versions.


I am not sure I follow this. The allocation deciders will not allow allocating replicas to an older node. But I think this is about the primary, do we risk having the primary on an older version than the snapshot (and yes we do and I agree that it is rare). So perhaps it should just read "allocation primaries" and not "allocating replicas" in this line?

henningandersen

LGTM.

henningandersen · 2021-10-12T07:08:51Z

...nternalClusterTest/java/org/elasticsearch/indices/recovery/SnapshotBasedIndexRecoveryIT.java

+                // Flush to ensure that index_commit_seq_nos(replica) == index_commit_seq_nos(primary),
+                // since the primary flushes the index before taking the snapshot.
+                flush(indexName);


I am not sure I follow this comment since at this time there is no replica?

henningandersen · 2021-10-12T07:15:41Z

...nternalClusterTest/java/org/elasticsearch/indices/recovery/SnapshotBasedIndexRecoveryIT.java

-            );
+            final Settings.Builder indexSettings = Settings.builder()
+                .put(IndexMetadata.SETTING_NUMBER_OF_SHARDS, 1)
+                .put(IndexMetadata.SETTING_NUMBER_OF_REPLICAS, 0)


Did you intend to add a replica in this test for the seqNoRecovery case? Running the test failed for me when looking up the replica shard further down.

henningandersen · 2021-10-12T07:21:20Z

server/src/main/java/org/elasticsearch/indices/recovery/RecoverySettings.java

    public static final Version SNAPSHOT_RECOVERIES_SUPPORTED_VERSION = Version.V_7_15_0;
-    public static final Version SEQ_NO_SNAPSHOT_RECOVERIES_SUPPORTED_VERSION = Version.V_7_16_0;
+    // TODO: update after backport
+    public static final Version SEQ_NO_SNAPSHOT_RECOVERIES_SUPPORTED_VERSION = Version.CURRENT;


nit: I would normally put in V_8_0_0 until backport here, but it is not important.

henningandersen · 2021-10-12T07:28:20Z

server/src/main/java/org/elasticsearch/index/shard/RemoveCorruptedShardDataCommand.java


            // commit the new history id
            userData.put(Engine.HISTORY_UUID_KEY, historyUUID);
+            userData.put(Engine.ES_VERSION, Version.CURRENT.toString());


I wonder if we should check that the version does not go backwards explicitly here. I wonder if someone might be able to apply the command from an earlier version in edge cases.

henningandersen · 2021-10-12T07:35:24Z

...nternalClusterTest/java/org/elasticsearch/indices/recovery/SnapshotBasedIndexRecoveryIT.java

+        List<IndexCommit> commits = DirectoryReader.listCommits(shard.store().directory());
+        IndexCommit safeCommit = CombinedDeletionPolicy.findSafeCommitPoint(commits, globalCheckpoint);
+        assertThat(safeCommit, is(notNullValue()));


Can we use IndexShard.acquireSafeCommit() instead? Need to release it then, but seems more straighforward.

henningandersen · 2021-10-12T07:42:57Z

...nternalClusterTest/java/org/elasticsearch/indices/recovery/SnapshotBasedIndexRecoveryIT.java

-        } finally {
-            updateSetting(INDICES_RECOVERY_MAX_CONCURRENT_SNAPSHOT_FILE_DOWNLOADS.getKey(), null);
-        }
+    private Store.MetadataSnapshot getMetadataSnapshot(String nodeName, String indexName, int globalCheckpoint) throws IOException {


It is not entirely clear to me why we need the global checkpoint passed in here rather than just take it from the shard.

fcofdez · 2021-10-13T13:49:06Z

@elasticmachine update branch

fcofdez · 2021-10-14T06:43:01Z

@elasticmachine update branch

This commit adds support for peer recoveries using snapshots after a primary failover if the snapshot shares the same logical contents but the physical files are different. It uses the seq no information stored in the snapshot to compare against the current shard source node seq nos and decide whether or not it can use the snapshot to recover the shard. Since the underlying index files are different to the source index files, error handling is different than when the files are shared. In this case, if there's an error while snapshots files are recovered, we have to cancel the on-going downloads, wait until all in-flight operations complete, remove the recovered files and start from scratch using a fallback recovery plan that uses the files from the source node. Relates elastic#73496 Backport of elastic#77420

fcofdez · 2021-10-14T11:29:14Z

Thanks for the review Henning!

…ailovers (#79137) This commit adds support for peer recoveries using snapshots after a primary failover if the snapshot shares the same logical contents but the physical files are different. It uses the seq no information stored in the snapshot to compare against the current shard source node seq nos and decide whether or not it can use the snapshot to recover the shard. Since the underlying index files are different to the source index files, error handling is different than when the files are shared. In this case, if there's an error while snapshots files are recovered, we have to cancel the on-going downloads, wait until all in-flight operations complete, remove the recovered files and start from scratch using a fallback recovery plan that uses the files from the source node. Relates #73496 Backport of #77420

…to 7.x

…79144)

…otIsUsedEvenIfFilesAreDifferent` (#114821) Don't test any 7.x snapshots, keep using any 8,x compatible snapshot and Lucene version. Originally added in 8.0 (#77420) for testing peer recoveries using snapshots. Co-authored-by: Yang Wang <ywangd@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…otIsUsedEvenIfFilesAreDifferent` (elastic#114821) Don't test any 7.x snapshots, keep using any 8,x compatible snapshot and Lucene version. Originally added in 8.0 (elastic#77420) for testing peer recoveries using snapshots. Co-authored-by: Yang Wang <ywangd@gmail.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

fcofdez added >enhancement :Distributed/Recovery Anything around constructing a new shard, either from a local or a remote source. v8.0.0 Team:Distributed Meta label for distributed team. v7.16.0 labels Sep 8, 2021

fcofdez mentioned this pull request Sep 8, 2021

Replicate or relocate data via snapshot #73496

Closed

8 tasks

fcofdez force-pushed the recover-seqno-snapshot branch from 3a05364 to fdd32d4 Compare September 8, 2021 12:40

fcofdez requested review from DaveCTurner and henningandersen September 8, 2021 13:27

fcofdez commented Sep 8, 2021

View reviewed changes

henningandersen reviewed Sep 8, 2021

View reviewed changes

fcofdez added 2 commits September 9, 2021 12:02

Avoid recursive call

e48a0cb

Merge remote-tracking branch 'origin/master' into recover-seqno-snapshot

c619f75

henningandersen mentioned this pull request Sep 9, 2021

Version aware restore allocation #77498

Open

fcofdez added 4 commits September 9, 2021 18:08

Add version checks

9f07fde

Merge remote-tracking branch 'origin/master' into recover-seqno-snapshot

1498d8a

Add version checks

27b0366

Merge remote-tracking branch 'origin/master' into recover-seqno-snapshot

ad0d94d

fcofdez requested a review from henningandersen September 10, 2021 10:31

henningandersen reviewed Sep 13, 2021

View reviewed changes

fcofdez added 3 commits September 14, 2021 10:01

Merge remote-tracking branch 'origin/master' into recover-seqno-snapshot

75c9f1d

Review comments

3c7d737

Update test

81dad90

fcofdez requested a review from henningandersen September 14, 2021 12:23

henningandersen reviewed Sep 20, 2021

View reviewed changes

fcofdez added 2 commits September 21, 2021 14:52

Merge remote-tracking branch 'origin/master' into recover-seqno-snapshot

443ee92

Review comments

5d1aea4

Adjust version

de15459

fcofdez requested a review from henningandersen September 29, 2021 09:26

henningandersen approved these changes Oct 12, 2021

View reviewed changes

fcofdez added 5 commits October 13, 2021 11:02

Merge remote-tracking branch 'origin/master' into recover-seqno-snapshot

e068a45

Minor comments

3b9626b

Fix tests

1c673ec

Fix test

ddd3b2a

Check for version in RemoveCorruptedShardDataCommand

076c710

Merge branch 'master' into recover-seqno-snapshot

2032b3b

Merge branch 'master' into recover-seqno-snapshot

9b42f9c

fcofdez merged commit b50b81e into elastic:master Oct 14, 2021

fcofdez added auto-backport Automatically create backport pull requests when merged and removed auto-backport Automatically create backport pull requests when merged labels Oct 14, 2021

fcofdez mentioned this pull request Oct 14, 2021

[7.x] Add support for peer recoveries using snapshots after primary failovers #79137

Merged

fcofdez added a commit to fcofdez/elasticsearch that referenced this pull request Oct 14, 2021

Mute bwc tests for backporting elastic#77420

a5f62f3

fcofdez added a commit that referenced this pull request Oct 14, 2021

Mute bwc tests for backporting #77420 (#79143)

f6ccdbd

fcofdez added a commit to fcofdez/elasticsearch that referenced this pull request Oct 14, 2021

Re-enable BWC tests and adjust versions after elastic#77420 backport …

9cbdf85

…to 7.x

fcofdez added a commit that referenced this pull request Oct 14, 2021

Re-enable BWC tests and adjust versions after #77420 backport to 7.x (#…

8b5d5ff

…79144)

jakelandis added v8.0.0-beta1 and removed v8.0.0 labels Oct 27, 2021

arteam mentioned this pull request Oct 16, 2024

[test] Don't test any 7.x snapshots in testLogicallyEquivalentSnapshotIsUsedEvenIfFilesAreDifferent #114821

Merged


		ShardSnapshot latestSnapshot = latestSnapshotOpt.get();

		if (latestSnapshot.isLogicallyEquivalent(shardStateIdentifier)) {

	snapshotVersion != null && snapshotVersion.onOrBefore(Version.CURRENT) &&
	(snapshotVersion == null \|\| snapshotVersion.onOrBefore(Version.CURRENT)) &&

	// the version that was used to write those Lucene segments
	// the version that was used to write the commit (and thus a max version for the underlying segments).

	return Version.CURRENT.luceneVersion.onOrAfter(snapshot.getCommitLuceneVersion());
	assert Version.V_7_16_0.luceneVersion.onOrAfter(snapshot.getCommitLuceneVersion());
	return Version.CURRENT.luceneVersion.onOrAfter(snapshot.getCommitLuceneVersion());

Conversation

fcofdez commented Sep 8, 2021

Uh oh!

elasticmachine commented Sep 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fcofdez commented Sep 14, 2021

Uh oh!

henningandersen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!