diff --git a/CHANGES.txt b/CHANGES.txt index a59c00ba3f..ee5b955eed 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -120,6 +120,7 @@ * Duplicate the buffer before passing it to analyser in SASI operation (CASSANDRA-13512) * Properly evict pstmts from prepared statements cache (CASSANDRA-13641) Merged from 3.0: + * Randomize batchlog endpoint selection with only 1 or 2 racks (CASSANDRA-12884) * Fix digest calculation for counter cells (CASSANDRA-13750) * Fix ColumnDefinition.cellValueType() for non-frozen collection and change SSTabledump to use type.toJSONString() (CASSANDRA-13573) * Skip materialized view addition if the base table doesn't exist (CASSANDRA-13737) diff --git a/src/java/org/apache/cassandra/batchlog/BatchlogManager.java b/src/java/org/apache/cassandra/batchlog/BatchlogManager.java index 9d2867f1fe..9b0cedc5f3 100644 --- a/src/java/org/apache/cassandra/batchlog/BatchlogManager.java +++ b/src/java/org/apache/cassandra/batchlog/BatchlogManager.java @@ -548,9 +548,14 @@ public class BatchlogManager implements BatchlogManagerMBean if (validated.keySet().size() == 1) { - // we have only 1 `other` rack - Collection otherRack = Iterables.getOnlyElement(validated.asMap().values()); - return Lists.newArrayList(Iterables.limit(otherRack, 2)); + /* + * we have only 1 `other` rack to select replicas from (whether it be the local rack or a single non-local rack) + * pick two random nodes from there; we are guaranteed to have at least two nodes in the single remaining rack + * because of the preceding if block. + */ + List otherRack = Lists.newArrayList(validated.values()); + shuffle(otherRack); + return otherRack.subList(0, 2); } // randomize which racks we pick from if more than 2 remaining @@ -562,7 +567,7 @@ public class BatchlogManager implements BatchlogManagerMBean else { racks = Lists.newArrayList(validated.keySet()); - Collections.shuffle((List) racks); + shuffle((List) racks); } // grab a random member of up to two racks @@ -587,5 +592,11 @@ public class BatchlogManager implements BatchlogManagerMBean { return ThreadLocalRandom.current().nextInt(bound); } + + @VisibleForTesting + protected void shuffle(List list) + { + Collections.shuffle(list); + } } } diff --git a/test/unit/org/apache/cassandra/batchlog/BatchlogEndpointFilterTest.java b/test/unit/org/apache/cassandra/batchlog/BatchlogEndpointFilterTest.java index 23aeaaa3ab..7db1cfa22c 100644 --- a/test/unit/org/apache/cassandra/batchlog/BatchlogEndpointFilterTest.java +++ b/test/unit/org/apache/cassandra/batchlog/BatchlogEndpointFilterTest.java @@ -20,7 +20,9 @@ package org.apache.cassandra.batchlog; import java.net.InetAddress; import java.net.UnknownHostException; import java.util.Collection; +import java.util.Collections; import java.util.HashSet; +import java.util.List; import com.google.common.collect.ImmutableMultimap; import com.google.common.collect.Multimap; @@ -87,8 +89,28 @@ public class BatchlogEndpointFilterTest .put("1", InetAddress.getByName("111")) .build(); Collection result = new TestEndpointFilter(LOCAL, endpoints).filter(); - // result should contain random two distinct values - assertThat(new HashSet<>(result).size(), is(2)); + // result should be the last two non-local replicas + // (Collections.shuffle has been replaced with Collections.reverse for testing) + assertThat(result.size(), is(2)); + assertThat(result, JUnitMatchers.hasItem(InetAddress.getByName("11"))); + assertThat(result, JUnitMatchers.hasItem(InetAddress.getByName("111"))); + } + + @Test + public void shouldSelectTwoRandomHostsFromSingleRack() throws UnknownHostException + { + Multimap endpoints = ImmutableMultimap. builder() + .put(LOCAL, InetAddress.getByName("1")) + .put(LOCAL, InetAddress.getByName("11")) + .put(LOCAL, InetAddress.getByName("111")) + .put(LOCAL, InetAddress.getByName("1111")) + .build(); + Collection result = new TestEndpointFilter(LOCAL, endpoints).filter(); + // result should be the last two non-local replicas + // (Collections.shuffle has been replaced with Collections.reverse for testing) + assertThat(result.size(), is(2)); + assertThat(result, JUnitMatchers.hasItem(InetAddress.getByName("111"))); + assertThat(result, JUnitMatchers.hasItem(InetAddress.getByName("1111"))); } private static class TestEndpointFilter extends BatchlogManager.EndpointFilter @@ -111,5 +133,12 @@ public class BatchlogEndpointFilterTest // We don't need random behavior here return bound - 1; } + + @Override + protected void shuffle(List list) + { + // We don't need random behavior here + Collections.reverse(list); + } } }