Skip to content

Commit 05ab45d

Browse files
github-actions[bot]github-actions[bot]
committed
Revert uploading of manifest using min codec version (#16403)
Signed-off-by: Sooraj Sinha <[email protected]> (cherry picked from commit 4ad1be3) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
1 parent 3493566 commit 05ab45d

File tree

8 files changed

+57
-115
lines changed

8 files changed

+57
-115
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
9898
- [Workload Management] Enhance rejection mechanism in workload management ([#16417](https://github.com/opensearch-project/OpenSearch/pull/16417))
9999
- [Workload Management] Fixing Create/Update QueryGroup TransportActions to execute from non-cluster manager nodes ([16422](https://github.com/opensearch-project/OpenSearch/pull/16422))
100100
- Fix flaky test in `testApproximateRangeWithSizeOverDefault` by adjusting totalHits assertion logic ([#16434](https://github.com/opensearch-project/OpenSearch/pull/16434#pullrequestreview-2386999409))
101+
- Revert changes to upload remote state manifest using minimum codec version([#16403](https://github.com/opensearch-project/OpenSearch/pull/16403))
101102

102103
### Security
103104

‎server/src/main/java/org/opensearch/cluster/coordination/PublicationTransportHandler.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -367,7 +367,6 @@ public PublicationContext newPublicationContext(
367367
}
368368

369369
private boolean validateRemotePublicationConfiguredOnAllNodes(DiscoveryNodes discoveryNodes) {
370-
assert ClusterMetadataManifest.getCodecForVersion(discoveryNodes.getMinNodeVersion()) >= ClusterMetadataManifest.CODEC_V0;
371370
for (DiscoveryNode node : discoveryNodes.getNodes().values()) {
372371
// if a node is non-remote then created local publication context
373372
if (node.isRemoteStatePublicationEnabled() == false) {

‎server/src/main/java/org/opensearch/gateway/GatewayMetaState.java‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -754,12 +754,8 @@ public void setLastAcceptedState(ClusterState clusterState) {
754754
}
755755
try {
756756
final RemoteClusterStateManifestInfo manifestDetails;
757-
// Decide the codec version
758-
int codecVersion = ClusterMetadataManifest.getCodecForVersion(clusterState.nodes().getMinNodeVersion());
759-
assert codecVersion >= 0 : codecVersion;
760-
logger.info("codec version is {}", codecVersion);
761757

762-
if (shouldWriteFullClusterState(clusterState, codecVersion)) {
758+
if (shouldWriteFullClusterState(clusterState)) {
763759
final Optional<ClusterMetadataManifest> latestManifest = remoteClusterStateService.getLatestClusterMetadataManifest(
764760
clusterState.getClusterName().value(),
765761
clusterState.metadata().clusterUUID()
@@ -776,7 +772,7 @@ public void setLastAcceptedState(ClusterState clusterState) {
776772
clusterState.metadata().clusterUUID()
777773
);
778774
}
779-
manifestDetails = remoteClusterStateService.writeFullMetadata(clusterState, previousClusterUUID, codecVersion);
775+
manifestDetails = remoteClusterStateService.writeFullMetadata(clusterState, previousClusterUUID);
780776
} else {
781777
assert verifyManifestAndClusterState(lastAcceptedManifest, lastAcceptedState) == true
782778
: "Previous manifest and previous ClusterState are not in sync";
@@ -821,13 +817,11 @@ private boolean verifyManifestAndClusterState(ClusterMetadataManifest manifest,
821817
return true;
822818
}
823819

824-
private boolean shouldWriteFullClusterState(ClusterState clusterState, int codecVersion) {
825-
assert lastAcceptedManifest == null || lastAcceptedManifest.getCodecVersion() <= codecVersion;
820+
private boolean shouldWriteFullClusterState(ClusterState clusterState) {
826821
if (lastAcceptedState == null
827822
|| lastAcceptedManifest == null
828823
|| (remoteClusterStateService.isRemotePublicationEnabled() == false && lastAcceptedState.term() != clusterState.term())
829-
|| lastAcceptedManifest.getOpensearchVersion() != Version.CURRENT
830-
|| lastAcceptedManifest.getCodecVersion() != codecVersion) {
824+
|| lastAcceptedManifest.getOpensearchVersion() != Version.CURRENT) {
831825
return true;
832826
}
833827
return false;

‎server/src/main/java/org/opensearch/gateway/remote/RemoteClusterStateService.java‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -301,8 +301,7 @@ public RemoteClusterStateService(
301301
* @return A manifest object which contains the details of uploaded entity metadata.
302302
*/
303303
@Nullable
304-
public RemoteClusterStateManifestInfo writeFullMetadata(ClusterState clusterState, String previousClusterUUID, int codecVersion)
305-
throws IOException {
304+
public RemoteClusterStateManifestInfo writeFullMetadata(ClusterState clusterState, String previousClusterUUID) throws IOException {
306305
final long startTimeNanos = relativeTimeNanosSupplier.getAsLong();
307306
if (clusterState.nodes().isLocalNodeElectedClusterManager() == false) {
308307
logger.error("Local node is not elected cluster manager. Exiting");
@@ -342,8 +341,7 @@ public RemoteClusterStateManifestInfo writeFullMetadata(ClusterState clusterStat
342341
!remoteClusterStateValidationMode.equals(RemoteClusterStateValidationMode.NONE)
343342
? new ClusterStateChecksum(clusterState, threadpool)
344343
: null,
345-
false,
346-
codecVersion
344+
false
347345
);
348346

349347
final long durationMillis = TimeValue.nsecToMSec(relativeTimeNanosSupplier.getAsLong() - startTimeNanos);
@@ -551,8 +549,7 @@ public RemoteClusterStateManifestInfo writeIncrementalMetadata(
551549
!remoteClusterStateValidationMode.equals(RemoteClusterStateValidationMode.NONE)
552550
? new ClusterStateChecksum(clusterState, threadpool)
553551
: null,
554-
false,
555-
previousManifest.getCodecVersion()
552+
false
556553
);
557554

558555
final long durationMillis = TimeValue.nsecToMSec(relativeTimeNanosSupplier.getAsLong() - startTimeNanos);
@@ -1024,8 +1021,7 @@ public RemoteClusterStateManifestInfo markLastStateAsCommitted(
10241021
!remoteClusterStateValidationMode.equals(RemoteClusterStateValidationMode.NONE)
10251022
? new ClusterStateChecksum(clusterState, threadpool)
10261023
: null,
1027-
true,
1028-
previousManifest.getCodecVersion()
1024+
true
10291025
);
10301026
if (!previousManifest.isClusterUUIDCommitted() && committedManifestDetails.getClusterMetadataManifest().isClusterUUIDCommitted()) {
10311027
remoteClusterStateCleanupManager.deleteStaleClusterUUIDs(clusterState, committedManifestDetails.getClusterMetadataManifest());

‎server/src/main/java/org/opensearch/gateway/remote/RemoteManifestManager.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,7 @@ RemoteClusterStateManifestInfo uploadManifest(
100100
String previousClusterUUID,
101101
ClusterStateDiffManifest clusterDiffManifest,
102102
ClusterStateChecksum clusterStateChecksum,
103-
boolean committed,
104-
int codecVersion
103+
boolean committed
105104
) {
106105
synchronized (this) {
107106
ClusterMetadataManifest.Builder manifestBuilder = ClusterMetadataManifest.builder();
@@ -112,7 +111,7 @@ RemoteClusterStateManifestInfo uploadManifest(
112111
.opensearchVersion(Version.CURRENT)
113112
.nodeId(nodeId)
114113
.committed(committed)
115-
.codecVersion(codecVersion)
114+
.codecVersion(ClusterMetadataManifest.MANIFEST_CURRENT_CODEC_VERSION)
116115
.indices(uploadedMetadataResult.uploadedIndexMetadata)
117116
.previousClusterUUID(previousClusterUUID)
118117
.clusterUUIDCommitted(clusterState.metadata().clusterUUIDCommitted())

‎server/src/test/java/org/opensearch/cluster/coordination/CoordinationStateTests.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,6 @@
6767

6868
import static java.util.Collections.emptyMap;
6969
import static java.util.Collections.emptySet;
70-
import static org.opensearch.gateway.remote.ClusterMetadataManifest.MANIFEST_CURRENT_CODEC_VERSION;
7170
import static org.opensearch.gateway.remote.RemoteClusterStateService.REMOTE_PUBLICATION_SETTING_KEY;
7271
import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_CLUSTER_STATE_REPOSITORY_NAME_ATTRIBUTE_KEY;
7372
import static org.opensearch.node.remotestore.RemoteStoreNodeAttribute.REMOTE_STORE_REPOSITORY_SETTINGS_ATTRIBUTE_KEY_PREFIX;
@@ -962,7 +961,7 @@ public void testHandlePrePublishAndCommitWhenRemoteStateEnabled() throws IOExcep
962961
.previousClusterUUID(randomAlphaOfLength(10))
963962
.clusterUUIDCommitted(true)
964963
.build();
965-
when(remoteClusterStateService.writeFullMetadata(clusterState, previousClusterUUID, MANIFEST_CURRENT_CODEC_VERSION)).thenReturn(
964+
when(remoteClusterStateService.writeFullMetadata(clusterState, previousClusterUUID)).thenReturn(
966965
new RemoteClusterStateManifestInfo(manifest, "path/to/manifest")
967966
);
968967

@@ -975,8 +974,7 @@ public void testHandlePrePublishAndCommitWhenRemoteStateEnabled() throws IOExcep
975974

976975
final CoordinationState coordinationState = createCoordinationState(persistedStateRegistry, node1, remoteStateSettings());
977976
coordinationState.handlePrePublish(clusterState);
978-
Mockito.verify(remoteClusterStateService, Mockito.times(1))
979-
.writeFullMetadata(clusterState, previousClusterUUID, MANIFEST_CURRENT_CODEC_VERSION);
977+
Mockito.verify(remoteClusterStateService, Mockito.times(1)).writeFullMetadata(clusterState, previousClusterUUID);
980978
assertThat(persistedStateRegistry.getPersistedState(PersistedStateType.REMOTE).getLastAcceptedState(), equalTo(clusterState));
981979

982980
when(remoteClusterStateService.markLastStateAsCommitted(any(), any(), eq(false))).thenReturn(

‎server/src/test/java/org/opensearch/gateway/GatewayMetaStatePersistedStateTests.java‎

Lines changed: 14 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -759,7 +759,7 @@ public void testRemotePersistedState() throws IOException {
759759
final RemoteClusterStateService remoteClusterStateService = Mockito.mock(RemoteClusterStateService.class);
760760
final ClusterMetadataManifest manifest = ClusterMetadataManifest.builder().clusterTerm(1L).stateVersion(5L).build();
761761
final String previousClusterUUID = "prev-cluster-uuid";
762-
Mockito.when(remoteClusterStateService.writeFullMetadata(Mockito.any(), Mockito.any(), eq(MANIFEST_CURRENT_CODEC_VERSION)))
762+
Mockito.when(remoteClusterStateService.writeFullMetadata(Mockito.any(), Mockito.any()))
763763
.thenReturn(new RemoteClusterStateManifestInfo(manifest, "path/to/manifest"));
764764

765765
Mockito.when(remoteClusterStateService.writeIncrementalMetadata(Mockito.any(), Mockito.any(), Mockito.any()))
@@ -777,7 +777,7 @@ public void testRemotePersistedState() throws IOException {
777777
);
778778

779779
remotePersistedState.setLastAcceptedState(clusterState);
780-
Mockito.verify(remoteClusterStateService).writeFullMetadata(clusterState, previousClusterUUID, MANIFEST_CURRENT_CODEC_VERSION);
780+
Mockito.verify(remoteClusterStateService).writeFullMetadata(clusterState, previousClusterUUID);
781781

782782
assertThat(remotePersistedState.getLastAcceptedState(), equalTo(clusterState));
783783
assertThat(remotePersistedState.getCurrentTerm(), equalTo(clusterTerm));
@@ -789,8 +789,7 @@ public void testRemotePersistedState() throws IOException {
789789
);
790790

791791
remotePersistedState.setLastAcceptedState(secondClusterState);
792-
Mockito.verify(remoteClusterStateService, times(1))
793-
.writeFullMetadata(secondClusterState, previousClusterUUID, MANIFEST_CURRENT_CODEC_VERSION);
792+
Mockito.verify(remoteClusterStateService, times(1)).writeFullMetadata(secondClusterState, previousClusterUUID);
794793

795794
assertThat(remotePersistedState.getLastAcceptedState(), equalTo(secondClusterState));
796795
assertThat(remotePersistedState.getCurrentTerm(), equalTo(clusterTerm));
@@ -820,9 +819,9 @@ public void testRemotePersistedStateWithDifferentNodeConfiguration() throws IOEx
820819
.clusterTerm(1L)
821820
.stateVersion(5L)
822821
.codecVersion(CODEC_V1)
823-
.opensearchVersion(Version.CURRENT)
822+
.opensearchVersion(Version.V_2_15_0)
824823
.build();
825-
Mockito.when(remoteClusterStateService.writeFullMetadata(Mockito.any(), Mockito.any(), eq(CODEC_V1)))
824+
Mockito.when(remoteClusterStateService.writeFullMetadata(Mockito.any(), Mockito.any()))
826825
.thenReturn(new RemoteClusterStateManifestInfo(manifest, "path/to/manifest2"));
827826

828827
CoordinationState.PersistedState remotePersistedState = new RemotePersistedState(remoteClusterStateService, previousClusterUUID);
@@ -833,7 +832,7 @@ public void testRemotePersistedStateWithDifferentNodeConfiguration() throws IOEx
833832
);
834833
remotePersistedState.setLastAcceptedState(clusterState1);
835834

836-
Mockito.verify(remoteClusterStateService).writeFullMetadata(clusterState1, previousClusterUUID, CODEC_V1);
835+
Mockito.verify(remoteClusterStateService).writeFullMetadata(clusterState1, previousClusterUUID);
837836

838837
ClusterState clusterState2 = createClusterState(
839838
randomNonNegativeLong(),
@@ -846,10 +845,10 @@ public void testRemotePersistedStateWithDifferentNodeConfiguration() throws IOEx
846845
.codecVersion(MANIFEST_CURRENT_CODEC_VERSION)
847846
.opensearchVersion(Version.CURRENT)
848847
.build();
849-
Mockito.when(remoteClusterStateService.writeFullMetadata(Mockito.any(), Mockito.any(), eq(MANIFEST_CURRENT_CODEC_VERSION)))
848+
Mockito.when(remoteClusterStateService.writeFullMetadata(Mockito.any(), Mockito.any()))
850849
.thenReturn(new RemoteClusterStateManifestInfo(manifest2, "path/to/manifest"));
851850
remotePersistedState.setLastAcceptedState(clusterState2);
852-
Mockito.verify(remoteClusterStateService).writeFullMetadata(clusterState2, previousClusterUUID, MANIFEST_CURRENT_CODEC_VERSION);
851+
Mockito.verify(remoteClusterStateService).writeFullMetadata(clusterState2, previousClusterUUID);
853852

854853
ClusterState clusterState3 = createClusterState(
855854
randomNonNegativeLong(),
@@ -889,8 +888,7 @@ public void testRemotePersistentState_FollowerNode() throws IOException {
889888

890889
remotePersistedState.setLastAcceptedState(clusterState);
891890
remotePersistedState.setLastAcceptedManifest(manifest);
892-
Mockito.verify(remoteClusterStateService, never())
893-
.writeFullMetadata(clusterState, previousClusterUUID, MANIFEST_CURRENT_CODEC_VERSION);
891+
Mockito.verify(remoteClusterStateService, never()).writeFullMetadata(clusterState, previousClusterUUID);
894892

895893
assertEquals(clusterState, remotePersistedState.getLastAcceptedState());
896894
assertEquals(clusterTerm, remotePersistedState.getCurrentTerm());
@@ -906,8 +904,7 @@ public void testRemotePersistentState_FollowerNode() throws IOException {
906904
);
907905

908906
remotePersistedState.setLastAcceptedState(secondClusterState);
909-
Mockito.verify(remoteClusterStateService, never())
910-
.writeFullMetadata(secondClusterState, previousClusterUUID, MANIFEST_CURRENT_CODEC_VERSION);
907+
Mockito.verify(remoteClusterStateService, never()).writeFullMetadata(secondClusterState, previousClusterUUID);
911908

912909
assertEquals(secondClusterState, remotePersistedState.getLastAcceptedState());
913910
assertEquals(clusterTerm, remotePersistedState.getCurrentTerm());
@@ -940,7 +937,7 @@ public void testRemotePersistedStateNotCommitted() throws IOException {
940937
.build();
941938
Mockito.when(remoteClusterStateService.getLatestClusterMetadataManifest(Mockito.any(), Mockito.any()))
942939
.thenReturn(Optional.of(manifest));
943-
Mockito.when(remoteClusterStateService.writeFullMetadata(Mockito.any(), Mockito.any(), eq(MANIFEST_CURRENT_CODEC_VERSION)))
940+
Mockito.when(remoteClusterStateService.writeFullMetadata(Mockito.any(), Mockito.any()))
944941
.thenReturn(new RemoteClusterStateManifestInfo(manifest, "path/to/manifest"));
945942

946943
Mockito.when(remoteClusterStateService.writeIncrementalMetadata(Mockito.any(), Mockito.any(), Mockito.any()))
@@ -966,17 +963,14 @@ public void testRemotePersistedStateNotCommitted() throws IOException {
966963
remotePersistedState.setLastAcceptedState(clusterState);
967964
ArgumentCaptor<String> previousClusterUUIDCaptor = ArgumentCaptor.forClass(String.class);
968965
ArgumentCaptor<ClusterState> clusterStateCaptor = ArgumentCaptor.forClass(ClusterState.class);
969-
Mockito.verify(remoteClusterStateService)
970-
.writeFullMetadata(clusterStateCaptor.capture(), previousClusterUUIDCaptor.capture(), eq(MANIFEST_CURRENT_CODEC_VERSION));
966+
Mockito.verify(remoteClusterStateService).writeFullMetadata(clusterStateCaptor.capture(), previousClusterUUIDCaptor.capture());
971967
assertEquals(previousClusterUUID, previousClusterUUIDCaptor.getValue());
972968
}
973969

974970
public void testRemotePersistedStateExceptionOnFullStateUpload() throws IOException {
975971
final RemoteClusterStateService remoteClusterStateService = Mockito.mock(RemoteClusterStateService.class);
976972
final String previousClusterUUID = "prev-cluster-uuid";
977-
Mockito.doThrow(IOException.class)
978-
.when(remoteClusterStateService)
979-
.writeFullMetadata(Mockito.any(), Mockito.any(), eq(MANIFEST_CURRENT_CODEC_VERSION));
973+
Mockito.doThrow(IOException.class).when(remoteClusterStateService).writeFullMetadata(Mockito.any(), Mockito.any());
980974

981975
CoordinationState.PersistedState remotePersistedState = new RemotePersistedState(remoteClusterStateService, previousClusterUUID);
982976

@@ -994,9 +988,7 @@ public void testRemotePersistedStateFailureStats() throws IOException {
994988
RemoteUploadStats remoteStateStats = new RemoteUploadStats();
995989
final RemoteClusterStateService remoteClusterStateService = Mockito.mock(RemoteClusterStateService.class);
996990
final String previousClusterUUID = "prev-cluster-uuid";
997-
Mockito.doThrow(IOException.class)
998-
.when(remoteClusterStateService)
999-
.writeFullMetadata(Mockito.any(), Mockito.any(), eq(MANIFEST_CURRENT_CODEC_VERSION));
991+
Mockito.doThrow(IOException.class).when(remoteClusterStateService).writeFullMetadata(Mockito.any(), Mockito.any());
1000992
when(remoteClusterStateService.getUploadStats()).thenReturn(remoteStateStats);
1001993
doAnswer((i) -> {
1002994
remoteStateStats.stateFailed();

0 commit comments

Comments
 (0)