Fix possible infinite loop in create repair range patch by Jimmy MÃ¥rdell; reviewed by yukim for CASSANDRA-7983
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/aa7794c8 Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/aa7794c8 Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/aa7794c8 Branch: refs/heads/trunk Commit: aa7794c83fb2a876ce6613ba573cc4fd7472dc95 Parents: 6b8b8e0 Author: Jimmy MÃ¥rdell <[email protected]> Authored: Mon Sep 29 14:56:33 2014 -0500 Committer: Yuki Morishita <[email protected]> Committed: Tue Sep 30 09:13:01 2014 -0500 ---------------------------------------------------------------------- CHANGES.txt | 1 + .../cassandra/service/StorageService.java | 27 +++++++--- .../service/StorageServiceServerTest.java | 55 ++++++++++++++++++++ 3 files changed, 75 insertions(+), 8 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/aa7794c8/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index dd5f7dd..5902d75 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -25,6 +25,7 @@ * Make repair no-op when RF=1 (CASSANDRA-7864) * Fix NPE when table dropped during streaming (CASSANDRA-7946) * Fix wrong progress when streaming uncompressed (CASSANDRA-7878) + * Fix possible infinite loop in creating repair range (CASSANDRA-7983) Merged from 1.2: * Don't index tombstones (CASSANDRA-7828) http://git-wip-us.apache.org/repos/asf/cassandra/blob/aa7794c8/src/java/org/apache/cassandra/service/StorageService.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/service/StorageService.java b/src/java/org/apache/cassandra/service/StorageService.java index 11eb91d..43bc198 100644 --- a/src/java/org/apache/cassandra/service/StorageService.java +++ b/src/java/org/apache/cassandra/service/StorageService.java @@ -2514,22 +2514,33 @@ public class StorageService extends NotificationBroadcasterSupport implements IE * @return collection of ranges that match ring layout in TokenMetadata */ @SuppressWarnings("unchecked") - private Collection<Range<Token>> createRepairRangeFrom(String beginToken, String endToken) + @VisibleForTesting + Collection<Range<Token>> createRepairRangeFrom(String beginToken, String endToken) { Token parsedBeginToken = getPartitioner().getTokenFactory().fromString(beginToken); Token parsedEndToken = getPartitioner().getTokenFactory().fromString(endToken); - Deque<Range<Token>> repairingRange = new ArrayDeque<>(); // Break up given range to match ring layout in TokenMetadata - Token previous = tokenMetadata.getPredecessor(TokenMetadata.firstToken(tokenMetadata.sortedTokens(), parsedEndToken)); - while (parsedBeginToken.compareTo(previous) < 0) + ArrayList<Range<Token>> repairingRange = new ArrayList<>(); + + ArrayList<Token> tokens = new ArrayList<>(tokenMetadata.sortedTokens()); + if (!tokens.contains(parsedBeginToken)) + { + tokens.add(parsedBeginToken); + } + if (!tokens.contains(parsedEndToken)) { - repairingRange.addFirst(new Range<>(previous, parsedEndToken)); + tokens.add(parsedEndToken); + } + // tokens now contain all tokens including our endpoints + Collections.sort(tokens); - parsedEndToken = previous; - previous = tokenMetadata.getPredecessor(previous); + int start = tokens.indexOf(parsedBeginToken), end = tokens.indexOf(parsedEndToken); + for (int i = start; i != end; i = (i+1) % tokens.size()) + { + Range<Token> range = new Range<>(tokens.get(i), tokens.get((i+1) % tokens.size())); + repairingRange.add(range); } - repairingRange.addFirst(new Range<>(parsedBeginToken, parsedEndToken)); return repairingRange; } http://git-wip-us.apache.org/repos/asf/cassandra/blob/aa7794c8/test/unit/org/apache/cassandra/service/StorageServiceServerTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/service/StorageServiceServerTest.java b/test/unit/org/apache/cassandra/service/StorageServiceServerTest.java index 503a730..d78c7d6 100644 --- a/test/unit/org/apache/cassandra/service/StorageServiceServerTest.java +++ b/test/unit/org/apache/cassandra/service/StorageServiceServerTest.java @@ -26,6 +26,10 @@ import java.util.*; import com.google.common.collect.HashMultimap; import com.google.common.collect.Multimap; + +import org.apache.cassandra.dht.BigIntegerToken; +import org.apache.cassandra.dht.LongToken; +import org.apache.cassandra.dht.Murmur3Partitioner; import org.junit.BeforeClass; import org.junit.Test; import org.junit.runner.RunWith; @@ -263,4 +267,55 @@ public class StorageServiceServerTest assert primaryRanges.size() == 1; assert primaryRanges.contains(new Range<Token>(new StringToken("B"), new StringToken("C"))); } + + @Test + public void testCreateRepairRangeFrom() throws Exception + { + StorageService.instance.setPartitionerUnsafe(new Murmur3Partitioner()); + + TokenMetadata metadata = StorageService.instance.getTokenMetadata(); + metadata.clearUnsafe(); + + metadata.updateNormalToken(new LongToken(1000L), InetAddress.getByName("127.0.0.1")); + metadata.updateNormalToken(new LongToken(2000L), InetAddress.getByName("127.0.0.2")); + metadata.updateNormalToken(new LongToken(3000L), InetAddress.getByName("127.0.0.3")); + metadata.updateNormalToken(new LongToken(4000L), InetAddress.getByName("127.0.0.4")); + + Map<String, String> configOptions = new HashMap<String, String>(); + configOptions.put("replication_factor", "3"); + + Keyspace.clear("Keyspace1"); + KSMetaData meta = KSMetaData.newKeyspace("Keyspace1", "SimpleStrategy", configOptions, false); + Schema.instance.setKeyspaceDefinition(meta); + + Collection<Range<Token>> repairRangeFrom = StorageService.instance.createRepairRangeFrom("1500", "3700"); + assert repairRangeFrom.size() == 3; + assert repairRangeFrom.contains(new Range<Token>(new LongToken(1500L), new LongToken(2000L))); + assert repairRangeFrom.contains(new Range<Token>(new LongToken(2000L), new LongToken(3000L))); + assert repairRangeFrom.contains(new Range<Token>(new LongToken(3000L), new LongToken(3700L))); + + repairRangeFrom = StorageService.instance.createRepairRangeFrom("500", "700"); + assert repairRangeFrom.size() == 1; + assert repairRangeFrom.contains(new Range<Token>(new LongToken(500L), new LongToken(700L))); + + repairRangeFrom = StorageService.instance.createRepairRangeFrom("500", "1700"); + assert repairRangeFrom.size() == 2; + assert repairRangeFrom.contains(new Range<Token>(new LongToken(500L), new LongToken(1000L))); + assert repairRangeFrom.contains(new Range<Token>(new LongToken(1000L), new LongToken(1700L))); + + repairRangeFrom = StorageService.instance.createRepairRangeFrom("2500", "2300"); + assert repairRangeFrom.size() == 5; + assert repairRangeFrom.contains(new Range<Token>(new LongToken(2500L), new LongToken(3000L))); + assert repairRangeFrom.contains(new Range<Token>(new LongToken(3000L), new LongToken(4000L))); + assert repairRangeFrom.contains(new Range<Token>(new LongToken(4000L), new LongToken(1000L))); + assert repairRangeFrom.contains(new Range<Token>(new LongToken(1000L), new LongToken(2000L))); + assert repairRangeFrom.contains(new Range<Token>(new LongToken(2000L), new LongToken(2300L))); + + repairRangeFrom = StorageService.instance.createRepairRangeFrom("2000", "3000"); + assert repairRangeFrom.size() == 1; + assert repairRangeFrom.contains(new Range<Token>(new LongToken(2000L), new LongToken(3000L))); + + repairRangeFrom = StorageService.instance.createRepairRangeFrom("2000", "2000"); + assert repairRangeFrom.size() == 0; + } }
