Skip to content

Commit fdc4e15

Browse files
soosinhasoosinha
committed
Revert uploading of manifest using min codec version
Signed-off-by: Sooraj Sinha <[email protected]>
1 parent bb1359f commit fdc4e15

File tree

8 files changed

+57
-115
lines changed

8 files changed

+57
-115
lines changed

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
9595
- Fix missing fields in task index mapping to ensure proper task result storage ([#16201](https://github.com/opensearch-project/OpenSearch/pull/16201))
9696
- Fix typo super->sb in method toString() of RemoteStoreNodeAttribute ([#15362](https://github.com/opensearch-project/OpenSearch/pull/15362))
9797
- [Workload Management] Fixing Create/Update QueryGroup TransportActions to execute from non-cluster manager nodes ([16422](https://github.com/opensearch-project/OpenSearch/pull/16422))
98+
- Revert changes to upload remote state manifest using minimum codec version([#16403](https://github.com/opensearch-project/OpenSearch/pull/16403))
9899

99100
### Security
100101

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

-1
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

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

761-
if (shouldWriteFullClusterState(clusterState, codecVersion)) {
757+
if (shouldWriteFullClusterState(clusterState)) {
762758
final Optional<ClusterMetadataManifest> latestManifest = remoteClusterStateService.getLatestClusterMetadataManifest(
763759
clusterState.getClusterName().value(),
764760
clusterState.metadata().clusterUUID()
@@ -775,7 +771,7 @@ public void setLastAcceptedState(ClusterState clusterState) {
775771
clusterState.metadata().clusterUUID()
776772
);
777773
}
778-
manifestDetails = remoteClusterStateService.writeFullMetadata(clusterState, previousClusterUUID, codecVersion);
774+
manifestDetails = remoteClusterStateService.writeFullMetadata(clusterState, previousClusterUUID);
779775
} else {
780776
assert verifyManifestAndClusterState(lastAcceptedManifest, lastAcceptedState) == true
781777
: "Previous manifest and previous ClusterState are not in sync";
@@ -820,13 +816,11 @@ private boolean verifyManifestAndClusterState(ClusterMetadataManifest manifest,
820816
return true;
821817
}
822818

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

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

+4-8
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

+2-3
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

+2-4
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

+14-22
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)