JVMCBC-1543 ViewLocator should use NodeIdentifier instead of hostname

Motivation ---------- The ViewLocator was using just a node's host to decide whether it can service view queries. This was potentially inaccurate, since multiple nodes can run on the same host. Apart from the inaccuracy, this strategy no longer works now that alternate addresses are resolved when the config is parsed. Now is the time to use a more robust strategy. Modifications ------------- Compare NodeIdentifiers instead of just hosts. Change-Id: Iaaf44604d9fdf43eb28441c3a248fdc4a5daa9c6 Reviewed-on: https://review.couchbase.org/c/couchbase-jvm-clients/+/212915 Reviewed-by: Michael Reiche <[email protected]> Tested-by: Build Bot <[email protected]>
couchbase · Jul 18, 2024 · d5061ae · d5061ae
1 parent c30b940
commit d5061ae
Show file tree

Hide file tree

Showing 5 changed files with 29 additions and 11 deletions.
diff --git a/core-io/src/main/java/com/couchbase/client/core/config/CouchbaseBucketConfig.java b/core-io/src/main/java/com/couchbase/client/core/config/CouchbaseBucketConfig.java
@@ -22,6 +22,7 @@
 import com.couchbase.client.core.deps.com.fasterxml.jackson.annotation.JsonIgnoreProperties;
 import com.couchbase.client.core.deps.com.fasterxml.jackson.annotation.JsonProperty;
 import com.couchbase.client.core.error.ConfigException;
+import com.couchbase.client.core.node.NodeIdentifier;
 import com.couchbase.client.core.service.ServiceType;
 import com.couchbase.client.core.topology.ClusterTopologyWithBucket;
 import com.couchbase.client.core.topology.CouchbaseBucketTopology;
@@ -58,7 +59,7 @@ public class CouchbaseBucketConfig extends AbstractBucketConfig {
     private final int numberOfPartitions;
 
     private final List<NodeInfo> partitionHosts;
-    private final Set<String> hostsWithPrimaryPartitions;
+    private final Set<NodeIdentifier> hostsWithPrimaryPartitions;
 
     private final List<Partition> partitions;
     private final List<Partition> forwardPartitions;
@@ -152,7 +153,7 @@ public CouchbaseBucketConfig(ClusterTopologyWithBucket cluster) {
 
         this.hostsWithPrimaryPartitions = new HashSet<>();
         bucket.partitions().values().forEach(partitionTopology ->
-          partitionTopology.active().ifPresent(it -> this.hostsWithPrimaryPartitions.add(it.host()))
+          partitionTopology.active().ifPresent(it -> this.hostsWithPrimaryPartitions.add(it.id().toLegacy()))
         );
     }
 
@@ -179,13 +180,15 @@ private static List<Partition> toLegacyPartitions(
      * @param partitions the partitions.
      * @return a set containing the addresses of nodes with primary partitions.
      */
-    private static Set<String> buildNodesWithPrimaryPartitions(final List<NodeInfo> nodeInfos,
-                                                               final List<Partition> partitions) {
-        Set<String> nodes = new HashSet<>(nodeInfos.size());
+    private static Set<NodeIdentifier> buildNodesWithPrimaryPartitions(
+        final List<NodeInfo> nodeInfos,
+        final List<Partition> partitions
+    ) {
+        Set<NodeIdentifier> nodes = new HashSet<>(nodeInfos.size());
         for (Partition partition : partitions) {
             int index = partition.active();
             if (index >= 0) {
-                nodes.add(nodeInfos.get(index).hostname());
+                nodes.add(nodeInfos.get(index).identifier());
             }
         }
         return nodes;
@@ -260,8 +263,17 @@ public boolean tainted() {
         return tainted;
     }
 
+    /**
+     * @deprecated In favor of {@link #hasPrimaryPartitionsOnNode(NodeIdentifier)}
+     * which handles the case where node hosts are not unique within the cluster.
+     */
+    @Deprecated
     public boolean hasPrimaryPartitionsOnNode(final String hostname) {
-        return hostsWithPrimaryPartitions.contains(hostname);
+        return hostsWithPrimaryPartitions.stream().anyMatch(it -> it.address().equals(hostname));
+    }
+
+    public boolean hasPrimaryPartitionsOnNode(final NodeIdentifier nodeId) {
+        return hostsWithPrimaryPartitions.contains(nodeId);
     }
 
     public short nodeIndexForActive(int partition, boolean useFastForward) {

diff --git a/core-io/src/main/java/com/couchbase/client/core/node/ViewLocator.java b/core-io/src/main/java/com/couchbase/client/core/node/ViewLocator.java
@@ -68,7 +68,7 @@ protected boolean nodeCanBeUsed(final Node node, final Request<? extends Respons
       BucketConfig bucketConfig = config.bucketConfig(bucket);
       if (bucketConfig instanceof CouchbaseBucketConfig) {
         return ((CouchbaseBucketConfig) bucketConfig)
-            .hasPrimaryPartitionsOnNode(node.identifier().address());
+            .hasPrimaryPartitionsOnNode(node.identifier());
       }
     }
     return false;

diff --git a/core-io/src/test/java/com/couchbase/client/core/config/CouchbaseBucketConfigTest.java b/core-io/src/test/java/com/couchbase/client/core/config/CouchbaseBucketConfigTest.java
@@ -17,6 +17,7 @@
 package com.couchbase.client.core.config;
 
 import com.couchbase.client.core.env.NetworkResolution;
+import com.couchbase.client.core.node.NodeIdentifier;
 import com.couchbase.client.core.node.StandardMemcachedHashingStrategy;
 import com.couchbase.client.core.service.ServiceType;
 import org.junit.jupiter.api.Test;
@@ -44,6 +45,8 @@ class CouchbaseBucketConfigTest {
     void shouldHavePrimaryPartitionsOnNode() {
         CouchbaseBucketConfig config = readConfig("config_with_mixed_partitions.json");
 
+        assertTrue(config.hasPrimaryPartitionsOnNode(new NodeIdentifier("1.2.3.4", 8091)));
+        assertFalse(config.hasPrimaryPartitionsOnNode(new NodeIdentifier("2.3.4.5", 8091)));
         assertTrue(config.hasPrimaryPartitionsOnNode("1.2.3.4"));
         assertFalse(config.hasPrimaryPartitionsOnNode("2.3.4.5"));
         assertEquals(BucketNodeLocator.VBUCKET, config.locator());

diff --git a/.../src/test/java/com/couchbase/client/core/config/CouchbaseBucketConfigTranslationTest.java b/.../src/test/java/com/couchbase/client/core/config/CouchbaseBucketConfigTranslationTest.java
@@ -70,6 +70,9 @@ public class CouchbaseBucketConfigTranslationTest {
   void shouldHavePrimaryPartitionsOnNode() {
     CouchbaseBucketConfig config = readConfig("config_7.6.0_2kv_but_only_1_has_active_partitions.json");
 
+    assertTrue(config.hasPrimaryPartitionsOnNode(new NodeIdentifier("1.2.3.4", 8091)));
+    assertFalse(config.hasPrimaryPartitionsOnNode(new NodeIdentifier("2.3.4.5", 8091)));
+
     assertTrue(config.hasPrimaryPartitionsOnNode("1.2.3.4"));
     assertFalse(config.hasPrimaryPartitionsOnNode("2.3.4.5"));
     assertEquals(BucketNodeLocator.VBUCKET, config.locator());

diff --git a/core-io/src/test/java/com/couchbase/client/core/node/ViewLocatorTest.java b/core-io/src/test/java/com/couchbase/client/core/node/ViewLocatorTest.java
@@ -36,8 +36,8 @@ void dispatchesOnlyToHostsWithPrimaryPartitionsEnabled() {
     CouchbaseBucketConfig bucketConfig = mock(CouchbaseBucketConfig.class);
     ClusterConfig config = mock(ClusterConfig.class);
     when(config.bucketConfig("bucket")).thenReturn(bucketConfig);
-    when(bucketConfig.hasPrimaryPartitionsOnNode("1.2.3.4")).thenReturn(true);
-    when(bucketConfig.hasPrimaryPartitionsOnNode("1.2.3.5")).thenReturn(false);
+    when(bucketConfig.hasPrimaryPartitionsOnNode(new NodeIdentifier("1.2.3.4", 1234))).thenReturn(true);
+    when(bucketConfig.hasPrimaryPartitionsOnNode(new NodeIdentifier("1.2.3.5", 1234))).thenReturn(false);
 
     Node node1 = mock(Node.class);
     when(node1.identifier()).thenReturn(new NodeIdentifier("1.2.3.4", 1234));
@@ -48,4 +48,4 @@ void dispatchesOnlyToHostsWithPrimaryPartitionsEnabled() {
     assertFalse(locator.nodeCanBeUsed(node2, request, config));
   }
 
-}
+}