Polishing #1909

Refactor BiFunction to isEqual BiPredicate. Return early if collection sizes do not match. Add tests and revise RedisClusterNode tests to verify equality and hashcode behavior.

Original pull request: #1912.

src/main/java/io/lettuce/core/cluster/RoundRobin.java

@@ -18,28 +18,29 @@
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
-import java.util.function.BiFunction;
+import java.util.function.BiPredicate;
 
 /**
  * Circular element provider. This class allows infinite scrolling over a collection with the possibility to provide an initial
  * offset.
  *
  * @author Mark Paluch
+ * @author Christian Lang
  */
 class RoundRobin<V> {
 
     protected volatile Collection<? extends V> collection = Collections.emptyList();
 
     protected volatile V offset;
 
-    private final BiFunction<V, V, Boolean> hasElementChanged;
+    private final BiPredicate<V, V> isEqual;
 
     public RoundRobin() {
-        this((a, b) -> false);
+        this((a, b) -> true);
     }
 
-    public RoundRobin(BiFunction<V, V, Boolean> hasElementChanged) {
-        this.hasElementChanged = hasElementChanged;
+    public RoundRobin(BiPredicate<V, V> hasElementChanged) {
+        this.isEqual = hasElementChanged;
     }
 
     /**
@@ -53,19 +54,31 @@ public boolean isConsistent(Collection<? extends V> leader) {
 
         Collection<? extends V> collection = this.collection;
 
+        if (collection.size() != leader.size()) {
+            return false;
+        }
+
         for (V currentElement : collection) {
-            boolean found = false;
-            for (V searchedElement : leader) {
-                if (searchedElement.equals(currentElement) && !hasElementChanged.apply(currentElement, searchedElement)) {
-                    found = true;
-                }
-            }
+
+            boolean found = find(leader, currentElement);
+
             if (!found) {
                 return false;
             }
         }
 
-        return collection.size() == leader.size();
+        return true;
+    }
+
+    private boolean find(Collection<? extends V> hayStack, V needle) {
+
+        for (V searchedElement : hayStack) {
+            if (searchedElement.equals(needle)) {
+                return isEqual.test(needle, searchedElement);
+            }
+        }
+
+        return false;
     }
 
     /**

src/main/java/io/lettuce/core/cluster/RoundRobinSocketAddressSupplier.java

@@ -31,6 +31,7 @@
  * Round-Robin socket address supplier. Cluster nodes are iterated circular/infinitely.
  *
  * @author Mark Paluch
+ * @author Christian Lang
  */
 class RoundRobinSocketAddressSupplier implements Supplier<SocketAddress> {
 
@@ -44,6 +45,7 @@ class RoundRobinSocketAddressSupplier implements Supplier<SocketAddress> {
 
     private final RoundRobin<RedisClusterNode> roundRobin;
 
+    @SuppressWarnings({ "unchecked", "rawtypes" })
     public RoundRobinSocketAddressSupplier(Supplier<Partitions> partitions,
             Function<? extends Collection<RedisClusterNode>, Collection<RedisClusterNode>> sortFunction,
             ClientResources clientResources) {
@@ -52,7 +54,8 @@ public RoundRobinSocketAddressSupplier(Supplier<Partitions> partitions,
         LettuceAssert.notNull(sortFunction, "Sort-Function must not be null");
 
         this.partitions = partitions;
-        this.roundRobin = new RoundRobin<>((a, b) -> !a.getUri().equals(b.getUri()));
+        this.roundRobin = new RoundRobin<>(
+                (l, r) -> l.getUri() == r.getUri() || (l.getUri() != null && l.getUri().equals(r.getUri())));
         this.sortFunction = (Function) sortFunction;
         this.clientResources = clientResources;
         resetRoundRobin(partitions.get());

src/test/java/io/lettuce/core/cluster/RoundRobinSocketAddressSupplierUnitTests.java

@@ -15,8 +15,8 @@
  */
 package io.lettuce.core.cluster;
 
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.Mockito.when;
+import static org.assertj.core.api.Assertions.*;
+import static org.mockito.Mockito.*;
 
 import java.net.InetSocketAddress;
 import java.time.Duration;
@@ -37,7 +37,10 @@
 import io.lettuce.core.resource.SocketAddressResolver;
 
 /**
+ * Unit tests for {@link RoundRobinSocketAddressSupplier}.
+ *
  * @author Mark Paluch
+ * @author Christian Lang
  */
 @ExtendWith(MockitoExtension.class)
 class RoundRobinSocketAddressSupplierUnitTests {

src/test/java/io/lettuce/core/cluster/RoundRobinUnitTests.java

@@ -0,0 +1,100 @@
+/*
+ * Copyright 2021 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.lettuce.core.cluster;
+
+import static org.assertj.core.api.Assertions.*;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashSet;
+
+import org.junit.jupiter.api.Test;
+
+import io.lettuce.core.RedisURI;
+import io.lettuce.core.cluster.models.partitions.RedisClusterNode;
+
+/**
+ * Unit tests for {@link RoundRobin}.
+ *
+ * @author Mark Paluch
+ */
+class RoundRobinUnitTests {
+
+    @Test
+    void shouldDetermineSimpleConsistency() {
+
+        RedisClusterNode node1 = new RedisClusterNode(RedisURI.create("127.0.0.1", 1), "1", true, "", 0, 0, 0,
+                new ArrayList<>(), new HashSet<>());
+        RedisClusterNode node2 = new RedisClusterNode(RedisURI.create("127.0.0.0", 1), "2", true, "", 0, 0, 0,
+                new ArrayList<>(), new HashSet<>());
+
+        RedisClusterNode newNode1 = new RedisClusterNode(RedisURI.create("127.0.0.0", 1), "1", true, "", 0, 0, 0,
+                new ArrayList<>(), new HashSet<>());
+
+        RoundRobin<RedisClusterNode> roundRobin = new RoundRobin<>();
+        roundRobin.rebuild(Arrays.asList(node1, node2));
+
+        assertThat(roundRobin.isConsistent(Arrays.asList(node1, node2))).isTrue();
+
+        // RedisClusterNode compares by Id only.
+        assertThat(roundRobin.isConsistent(Arrays.asList(newNode1, node2))).isTrue();
+        assertThat(roundRobin.isConsistent(Arrays.asList(RedisClusterNode.of("1"), node2))).isTrue();
+    }
+
+    @Test
+    void shouldDetermineConsistencyWithEqualityCheck() {
+
+        RedisClusterNode node1 = new RedisClusterNode(RedisURI.create("127.0.0.1", 1), "1", true, "", 0, 0, 0,
+                new ArrayList<>(), new HashSet<>());
+        RedisClusterNode node2 = new RedisClusterNode(RedisURI.create("127.0.0.0", 1), "2", true, "", 0, 0, 0,
+                new ArrayList<>(), new HashSet<>());
+        RedisClusterNode newNode1 = new RedisClusterNode(RedisURI.create("127.0.0.0", 1), "1", true, "", 0, 0, 0,
+                new ArrayList<>(), new HashSet<>());
+
+        RoundRobin<RedisClusterNode> roundRobin = new RoundRobin<>((l, r) -> l.getUri().equals(r.getUri()));
+        roundRobin.rebuild(Arrays.asList(node1, node2));
+
+        assertThat(roundRobin.isConsistent(Arrays.asList(node1, node2))).isTrue();
+
+        // RedisClusterNode compares by Id only.
+        assertThat(roundRobin.isConsistent(Arrays.asList(newNode1, node2))).isFalse();
+        assertThat(roundRobin.isConsistent(Collections.singletonList(newNode1))).isFalse();
+        assertThat(roundRobin.isConsistent(Collections.singletonList(node2))).isFalse();
+        assertThat(roundRobin.isConsistent(Arrays.asList(RedisClusterNode.of("1"), node2))).isFalse();
+    }
+
+    @Test
+    void shouldDetermineConsistencyWithEqualityCheckOppositeCheck() {
+
+        RedisClusterNode node1 = RedisClusterNode.of("1");
+        RedisClusterNode node2 = RedisClusterNode.of("2");
+        RedisClusterNode newNode1 = new RedisClusterNode(RedisURI.create("127.0.0.0", 1), "1", true, "", 0, 0, 0,
+                new ArrayList<>(), new HashSet<>());
+
+        RoundRobin<RedisClusterNode> roundRobin = new RoundRobin<>(
+                (l, r) -> l.getUri() == r.getUri() || (l.getUri() != null && l.getUri().equals(r.getUri())));
+        roundRobin.rebuild(Arrays.asList(node1, node2));
+
+        assertThat(roundRobin.isConsistent(Arrays.asList(node1, node2))).isTrue();
+
+        // RedisClusterNode compares by Id only.
+        assertThat(roundRobin.isConsistent(Arrays.asList(newNode1, node2))).isFalse();
+        assertThat(roundRobin.isConsistent(Collections.singletonList(newNode1))).isFalse();
+        assertThat(roundRobin.isConsistent(Collections.singletonList(node2))).isFalse();
+    }
+
+}

src/test/java/io/lettuce/core/cluster/models/partitions/RedisClusterNodeUnitTests.java

@@ -15,7 +15,7 @@
  */
 package io.lettuce.core.cluster.models.partitions;
 
-import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.*;
 
 import java.util.Arrays;
 
@@ -25,6 +25,8 @@
 import io.lettuce.core.cluster.SlotHash;
 
 /**
+ * Unit tests for {@link RedisClusterNode}.
+ *
  * @author Mark Paluch
  */
 class RedisClusterNodeUnitTests {
@@ -44,16 +46,20 @@ void shouldCopyNode() {
         assertThat(copy.getAliases()).contains(RedisURI.create("foo", 6379));
     }
 
-    @Test
+    @Test // considers nodeId only
     void testEquality() {
 
-        RedisClusterNode node = new RedisClusterNode();
+        RedisClusterNode node = RedisClusterNode.of("1");
+
+        assertThat(node).isEqualTo(RedisClusterNode.of("1"));
+        assertThat(node).hasSameHashCodeAs(RedisClusterNode.of("1"));
 
-        assertThat(node).isEqualTo(new RedisClusterNode());
-        assertThat(node.hashCode()).isEqualTo(new RedisClusterNode().hashCode());
+        node.setUri(RedisURI.create("127.0.0.1", 1));
+        assertThat(node).hasSameHashCodeAs(RedisClusterNode.of("1"));
+        assertThat(node).isEqualTo(RedisClusterNode.of("1"));
 
-        node.setUri(new RedisURI());
-        assertThat(node.hashCode()).isNotEqualTo(new RedisClusterNode());
+        assertThat(node).doesNotHaveSameHashCodeAs(RedisClusterNode.of("2"));
+        assertThat(node).isNotEqualTo(RedisClusterNode.of("2"));
     }
 
     @Test