Snapshot Stability Fixes #39502
Snapshot Stability Fixes #39502
Conversation
original-brownbear
added
>bug
:Distributed Coordination/Snapshot/Restore
|
Pinging @elastic/es-distributed |
| @@ -437,14 +455,21 @@ public SnapshotsInProgress(StreamInput in) throws IOException { | |||
| if (in.getVersion().onOrAfter(REPOSITORY_ID_INTRODUCED_VERSION)) { | |||
| repositoryStateId = in.readLong(); | |||
| } | |||
| final String failure; | |||
| if (in.getVersion().onOrAfter(Version.V_6_7_0)) { | |||
There was a problem hiding this comment.
We will have to mute BwC tests before merging this and then adjust this version to 6.7 in master afterwards, currently we have a 7.0.0 here in master.
| @@ -476,6 +501,9 @@ public void writeTo(StreamOutput out) throws IOException { | |||
| if (out.getVersion().onOrAfter(REPOSITORY_ID_INTRODUCED_VERSION)) { | |||
| out.writeLong(entry.repositoryStateId); | |||
| } | |||
| if (out.getVersion().onOrAfter(Version.V_6_7_0)) { | |||
There was a problem hiding this comment.
We will have to mute BwC tests before merging this and then adjust this version to 6.7 in master afterwards, currently we have a 7.0.0 here in master.
| UpdateSnapshotStatusAction(TransportService transportService, ClusterService clusterService, | ||
| ThreadPool threadPool, ActionFilters actionFilters, IndexNameExpressionResolver indexNameExpressionResolver) { | ||
| super( | ||
| settings, SnapshotShardsService.UPDATE_SNAPSHOT_STATUS_ACTION_NAME, transportService, clusterService, threadPool, |
There was a problem hiding this comment.
passing the settings here (we don't have that argument in master anymore) is the only difference between this PR and master in this file.
|
|
||
| // Set of snapshots that are currently being ended by this node | ||
| private final Set<Snapshot> endingSnapshots = Collections.synchronizedSet(new HashSet<>()); | ||
|
|
||
| @Inject | ||
| public SnapshotsService(Settings settings, ClusterService clusterService, IndexNameExpressionResolver indexNameExpressionResolver, |
There was a problem hiding this comment.
This class should be exactly like it is in master now
|
seems I missed a bwc spot, looking into it now |
* Backport of various snapshot stability fixes from `master` to `6.7` * Includes elastic#38368, elastic#38025 and elastic#37612
original-brownbear
force-pushed
the
backport-snapshot-fixes-6.7
branch
from
00266ba to
56b0f19
Compare
There was a problem hiding this comment.
@ywelsch Added some notes and fixed BwC here now, should be good to review :)
| final ShardId shardId, | ||
| final ShardSnapshotStatus status, | ||
| final DiscoveryNode masterNode) { | ||
| void sendSnapshotShardUpdate(Snapshot snapshot, ShardId shardId, ShardSnapshotStatus status, DiscoveryNode masterNode) { | ||
| try { | ||
| if (masterNode.getVersion().onOrAfter(Version.V_6_1_0)) { |
There was a problem hiding this comment.
I simply made it a hard condition here between the old and the new path, the old path doesn't use the de-duplicator.
I figured the optimization isn't really important in the rolling upgrade case and this makes future backports a lot easier than having the conditionals for that in the old code, right?
| @@ -452,14 +424,13 @@ private void syncShardStatsOnNewMaster(ClusterChangedEvent event) { | |||
| // but we think the shard is done - we need to make new master know that the shard is done | |||
| logger.debug("[{}] new master thinks the shard [{}] is not completed but the shard is done locally, " + | |||
| "updating status on the master", snapshot.snapshot(), shardId); | |||
| notifySuccessfulSnapshotShard(snapshot.snapshot(), shardId, localNodeId, masterNode); | |||
| notifySuccessfulSnapshotShard(snapshot.snapshot(), shardId, masterNode); | |||
There was a problem hiding this comment.
All this passing down of masterNode is different from the 7.x/master version since we need the Bwc path in the status update sending below.
|
@ywelsch thanks! |
This reverts commit 4b725e0.
masterto6.7making the snapshot logic in6.7equivalent to that inmasterfunctionally