From 686d1fe5252ede31b3d050623be7d9e976a7b676 Mon Sep 17 00:00:00 2001
From: Lakshya Taragi <lakshya.taragi@gmail.com>
Date: Thu, 18 Apr 2024 13:50:51 +0530
Subject: [PATCH] Address comments

Signed-off-by: Lakshya Taragi <lakshya.taragi@gmail.com>
---
 .../coordination/JoinTaskExecutor.java        | 41 ++++++++++---------
 .../coordination/JoinTaskExecutorTests.java   | 15 +++----
 2 files changed, 26 insertions(+), 30 deletions(-)

diff --git a/server/src/main/java/org/opensearch/cluster/coordination/JoinTaskExecutor.java b/server/src/main/java/org/opensearch/cluster/coordination/JoinTaskExecutor.java
index 05202d951cab4..5475470b81b93 100644
--- a/server/src/main/java/org/opensearch/cluster/coordination/JoinTaskExecutor.java
+++ b/server/src/main/java/org/opensearch/cluster/coordination/JoinTaskExecutor.java
@@ -31,6 +31,7 @@
 
 package org.opensearch.cluster.coordination;
 
+import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import org.opensearch.Version;
 import org.opensearch.cluster.ClusterState;
@@ -67,6 +68,7 @@
 import static org.opensearch.cluster.decommission.DecommissionHelper.nodeCommissioned;
 import static org.opensearch.gateway.GatewayService.STATE_NOT_RECOVERED_BLOCK;
 import static org.opensearch.node.remotestore.RemoteStoreNodeService.CompatibilityMode;
+import static org.opensearch.node.remotestore.RemoteStoreNodeService.CompatibilityMode.MIXED;
 import static org.opensearch.node.remotestore.RemoteStoreNodeService.CompatibilityMode.STRICT;
 import static org.opensearch.node.remotestore.RemoteStoreNodeService.REMOTE_STORE_COMPATIBILITY_MODE_SETTING;
 
@@ -79,7 +81,7 @@ public class JoinTaskExecutor implements ClusterStateTaskExecutor<JoinTaskExecut
 
     private final AllocationService allocationService;
 
-    private final Logger logger;
+    private static Logger logger = LogManager.getLogger(JoinTaskExecutor.class);
     private final RerouteService rerouteService;
 
     private final RemoteStoreNodeService remoteStoreNodeService;
@@ -143,7 +145,7 @@ public JoinTaskExecutor(
         RemoteStoreNodeService remoteStoreNodeService
     ) {
         this.allocationService = allocationService;
-        this.logger = logger;
+        JoinTaskExecutor.logger = logger;
         this.rerouteService = rerouteService;
         this.remoteStoreNodeService = remoteStoreNodeService;
     }
@@ -521,24 +523,23 @@ private static void ensureRemoteStoreNodesCompatibility(DiscoveryNode joiningNod
                     );
                 }
             }
-        } else if (remoteStoreCompatibilityMode == CompatibilityMode.MIXED) {
-            Version joiningNodeVersion = joiningNode.getVersion();
-            if (joiningNodeVersion.after(currentNodes.getMaxNodeVersion()) || joiningNodeVersion.before(currentNodes.getMinNodeVersion())) {
-                boolean isAfter = joiningNodeVersion.after(currentNodes.getMaxNodeVersion());
-                String reason = String.format(
-                    Locale.ROOT,
-                    "mixed mode: a %s version [%s] node [%s] is not allowed to join cluster with %s version [%s]",
-                    isAfter ? "higher" : "lower",
-                    joiningNode.getVersion(),
-                    joiningNode,
-                    isAfter ? "maximum" : "minimum",
-                    isAfter ? currentNodes.getMaxNodeVersion() : currentNodes.getMinNodeVersion()
-                );
-                throw new IllegalStateException(reason);
-            }
-            if (joiningNode.isRemoteStoreNode()) {
-                Optional<DiscoveryNode> remoteDN = existingNodes.stream().filter(DiscoveryNode::isRemoteStoreNode).findFirst();
-                remoteDN.ifPresent(discoveryNode -> ensureRemoteStoreNodesCompatibility(joiningNode, discoveryNode));
+        } else {
+            if (MIXED.equals(remoteStoreCompatibilityMode)) {
+                if (joiningNode.getVersion().after(currentNodes.getMaxNodeVersion())) {
+                    String reason = String.format(
+                        Locale.ROOT,
+                        "remote migration : a node [%s] of higher version [%s] is not allowed to join a cluster with maximum version [%s]",
+                        joiningNode,
+                        joiningNode.getVersion(),
+                        currentNodes.getMaxNodeVersion()
+                    );
+                    logger.warn(reason);
+                    throw new IllegalStateException(reason);
+                }
+                if (joiningNode.isRemoteStoreNode()) {
+                    Optional<DiscoveryNode> remoteDN = existingNodes.stream().filter(DiscoveryNode::isRemoteStoreNode).findFirst();
+                    remoteDN.ifPresent(discoveryNode -> ensureRemoteStoreNodesCompatibility(joiningNode, discoveryNode));
+                }
             }
         }
     }
diff --git a/server/src/test/java/org/opensearch/cluster/coordination/JoinTaskExecutorTests.java b/server/src/test/java/org/opensearch/cluster/coordination/JoinTaskExecutorTests.java
index b09a3f92a4143..3e343e95f6c4b 100644
--- a/server/src/test/java/org/opensearch/cluster/coordination/JoinTaskExecutorTests.java
+++ b/server/src/test/java/org/opensearch/cluster/coordination/JoinTaskExecutorTests.java
@@ -891,8 +891,7 @@ public void testNodeJoinInMixedMode() {
         FeatureFlags.initializeFeatureFlags(nodeSettings);
 
         List<Version> versions = allOpenSearchVersions();
-        assert versions.size() >= 3 : "test requires at least three open search versions";
-        Version lowerVersion = versions.get(versions.size() - 3);
+        assert versions.size() >= 2 : "test requires at least two open search versions";
         Version baseVersion = versions.get(versions.size() - 2);
         Version higherVersion = versions.get(versions.size() - 1);
 
@@ -911,16 +910,14 @@ public void testNodeJoinInMixedMode() {
 
         Metadata metadata = Metadata.builder().persistentSettings(mixedModeCompatibilitySettings).build();
 
-        boolean joiningNodeIsHigher = randomBoolean();
-
-        // joining node of a different version (higher or lower) than the current nodes
+        // joining node of a higher version than the current nodes
         DiscoveryNode joiningNode1 = new DiscoveryNode(
             randomAlphaOfLength(10),
             randomAlphaOfLength(10),
             buildNewFakeTransportAddress(),
             remoteStoreNodeAttributes(SEGMENT_REPO, TRANSLOG_REPO),
             Collections.singleton(DiscoveryNodeRole.CLUSTER_MANAGER_ROLE),
-            joiningNodeIsHigher ? higherVersion : lowerVersion
+            higherVersion
         );
         final IllegalStateException exception = expectThrows(
             IllegalStateException.class,
@@ -928,11 +925,9 @@ public void testNodeJoinInMixedMode() {
         );
         String reason = String.format(
             Locale.ROOT,
-            "mixed mode: a %s version [%s] node [%s] is not allowed to join cluster with %s version [%s]",
-            joiningNodeIsHigher ? "higher" : "lower",
-            joiningNode1.getVersion(),
+            "remote migration : a node [%s] of higher version [%s] is not allowed to join a cluster with maximum version [%s]",
             joiningNode1,
-            joiningNodeIsHigher ? "maximum" : "minimum",
+            joiningNode1.getVersion(),
             currentNodes.getMaxNodeVersion()
         );
         assertEquals(reason, exception.getMessage());