Repository: cassandra Updated Branches: refs/heads/cassandra-2.2 10be826e7 -> 3aa7308e8
Improve assertions around some of the usage of AbstractBounds patch by Sylvain Lebresne and Ariel Weisberg; reviewed by Aleksey Yeschenko for CASSANDRA-9462 Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/3aa7308e Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/3aa7308e Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/3aa7308e Branch: refs/heads/cassandra-2.2 Commit: 3aa7308e8f86969158c8d919c3f77658ae7c4fc3 Parents: 10be826 Author: Sylvain Lebresne <[email protected]> Authored: Wed Aug 12 16:34:07 2015 -0400 Committer: Aleksey Yeschenko <[email protected]> Committed: Fri Aug 14 21:20:17 2015 +0300 ---------------------------------------------------------------------- .../apache/cassandra/db/ColumnFamilyStore.java | 18 +++++++++++--- .../cassandra/db/SizeEstimatesRecorder.java | 3 ++- .../org/apache/cassandra/db/lifecycle/View.java | 13 +++++++--- .../apache/cassandra/dht/AbstractBounds.java | 25 ++++++++++++++++++++ src/java/org/apache/cassandra/dht/Bounds.java | 2 +- .../apache/cassandra/dht/ExcludingBounds.java | 2 +- .../cassandra/dht/IncludingExcludingBounds.java | 2 +- .../cassandra/streaming/StreamSession.java | 19 ++++++++------- .../apache/cassandra/db/lifecycle/ViewTest.java | 7 +++--- 9 files changed, 70 insertions(+), 21 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/3aa7308e/src/java/org/apache/cassandra/db/ColumnFamilyStore.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java index 482e3ee..8bda6b2 100644 --- a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java +++ b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java @@ -1929,11 +1929,17 @@ public class ColumnFamilyStore implements ColumnFamilyStoreMBean */ public Function<View, List<SSTableReader>> viewFilter(final AbstractBounds<RowPosition> rowBounds) { + assert !AbstractBounds.strictlyWrapsAround(rowBounds.left, rowBounds.right); return new Function<View, List<SSTableReader>>() { public List<SSTableReader> apply(View view) { - return compactionStrategyWrapper.filterSSTablesForReads(view.sstablesInBounds(rowBounds)); + // Note that View.sstablesInBounds always includes it's bound while rowBounds may not. This is ok however + // because the fact we restrict the sstables returned by this function is an optimization in the first + // place and the returned sstables will (almost) never cover *exactly* rowBounds anyway. It's also + // *very* unlikely that a sstable is included *just* because we consider one of the bound inclusively + // instead of exclusively, so the performance impact is negligible in practice. + return view.sstablesInBounds(rowBounds.left, rowBounds.right); } }; } @@ -1944,6 +1950,7 @@ public class ColumnFamilyStore implements ColumnFamilyStoreMBean */ public Function<View, List<SSTableReader>> viewFilter(final Collection<AbstractBounds<RowPosition>> rowBoundsCollection, final boolean includeRepaired) { + assert AbstractBounds.noneStrictlyWrapsAround(rowBoundsCollection); return new Function<View, List<SSTableReader>>() { public List<SSTableReader> apply(View view) @@ -1951,7 +1958,12 @@ public class ColumnFamilyStore implements ColumnFamilyStoreMBean Set<SSTableReader> sstables = Sets.newHashSet(); for (AbstractBounds<RowPosition> rowBounds : rowBoundsCollection) { - for (SSTableReader sstable : view.sstablesInBounds(rowBounds)) + // Note that View.sstablesInBounds always includes it's bound while rowBounds may not. This is ok however + // because the fact we restrict the sstables returned by this function is an optimization in the first + // place and the returned sstables will (almost) never cover *exactly* rowBounds anyway. It's also + // *very* unlikely that a sstable is included *just* because we consider one of the bound inclusively + // instead of exclusively, so the performance impact is negligible in practice. + for (SSTableReader sstable : view.sstablesInBounds(rowBounds.left, rowBounds.right)) { if (includeRepaired || !sstable.isRepaired()) sstables.add(sstable); @@ -2335,7 +2347,7 @@ public class ColumnFamilyStore implements ColumnFamilyStoreMBean { if (!manifestFile.getParentFile().exists()) manifestFile.getParentFile().mkdirs(); - + try (PrintStream out = new PrintStream(manifestFile)) { final JSONObject manifestJSON = new JSONObject(); http://git-wip-us.apache.org/repos/asf/cassandra/blob/3aa7308e/src/java/org/apache/cassandra/db/SizeEstimatesRecorder.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/SizeEstimatesRecorder.java b/src/java/org/apache/cassandra/db/SizeEstimatesRecorder.java index c68109c..f054315 100644 --- a/src/java/org/apache/cassandra/db/SizeEstimatesRecorder.java +++ b/src/java/org/apache/cassandra/db/SizeEstimatesRecorder.java @@ -85,9 +85,10 @@ public class SizeEstimatesRecorder extends MigrationListener implements Runnable @SuppressWarnings("resource") private void recordSizeEstimates(ColumnFamilyStore table, Collection<Range<Token>> localRanges) { + List<Range<Token>> unwrappedRanges = Range.normalize(localRanges); // for each local primary range, estimate (crudely) mean partition size and partitions count. Map<Range<Token>, Pair<Long, Long>> estimates = new HashMap<>(localRanges.size()); - for (Range<Token> range : localRanges) + for (Range<Token> range : unwrappedRanges) { // filter sstables that have partitions in this range. Refs<SSTableReader> refs = null; http://git-wip-us.apache.org/repos/asf/cassandra/blob/3aa7308e/src/java/org/apache/cassandra/db/lifecycle/View.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/lifecycle/View.java b/src/java/org/apache/cassandra/db/lifecycle/View.java index 0d1100b..73ba131 100644 --- a/src/java/org/apache/cassandra/db/lifecycle/View.java +++ b/src/java/org/apache/cassandra/db/lifecycle/View.java @@ -126,12 +126,19 @@ public class View return String.format("View(pending_count=%d, sstables=%s, compacting=%s)", liveMemtables.size() + flushingMemtables.size() - 1, sstables, compacting); } - public List<SSTableReader> sstablesInBounds(AbstractBounds<RowPosition> rowBounds) + /** + * Returns the sstables that have any partition between {@code left} and {@code right}, when both bounds are taken inclusively. + * The interval formed by {@code left} and {@code right} shouldn't wrap. + */ + public List<SSTableReader> sstablesInBounds(RowPosition left, RowPosition right) { + assert !AbstractBounds.strictlyWrapsAround(left, right); + if (intervalTree.isEmpty()) return Collections.emptyList(); - RowPosition stopInTree = rowBounds.right.isMinimum() ? intervalTree.max() : rowBounds.right; - return intervalTree.search(Interval.<RowPosition, SSTableReader>create(rowBounds.left, stopInTree)); + + RowPosition stopInTree = right.isMinimum() ? intervalTree.max() : right; + return intervalTree.search(Interval.<RowPosition, SSTableReader>create(left, stopInTree)); } // METHODS TO CONSTRUCT FUNCTIONS FOR MODIFYING A VIEW: http://git-wip-us.apache.org/repos/asf/cassandra/blob/3aa7308e/src/java/org/apache/cassandra/dht/AbstractBounds.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/dht/AbstractBounds.java b/src/java/org/apache/cassandra/dht/AbstractBounds.java index b5ffc22..c33ffc0 100644 --- a/src/java/org/apache/cassandra/dht/AbstractBounds.java +++ b/src/java/org/apache/cassandra/dht/AbstractBounds.java @@ -20,6 +20,7 @@ package org.apache.cassandra.dht; import java.io.DataInput; import java.io.IOException; import java.io.Serializable; +import java.util.Collection; import java.util.List; import org.apache.cassandra.db.DecoratedKey; @@ -71,6 +72,30 @@ public abstract class AbstractBounds<T extends RingPosition<T>> implements Seria public abstract boolean inclusiveLeft(); public abstract boolean inclusiveRight(); + /** + * Whether {@code left} and {@code right} forms a wrapping interval, that is if unwrapping wouldn't be a no-op. + * <p> + * Note that the semantic is slightly different from {@link Range#isWrapAround()} in the sense that if both + * {@code right} are minimal (for the partitioner), this methods return false (doesn't wrap) while + * {@link Range#isWrapAround()} returns true (does wrap). This is confusing and we should fix it by + * refactoring/rewriting the whole AbstractBounds hierarchy with cleaner semantics, but we don't want to risk + * breaking something by changing {@link Range#isWrapAround()} in the meantime. + */ + public static <T extends RingPosition<T>> boolean strictlyWrapsAround(T left, T right) + { + return !(left.compareTo(right) <= 0 || right.isMinimum()); + } + + public static <T extends RingPosition<T>> boolean noneStrictlyWrapsAround(Collection<AbstractBounds<T>> bounds) + { + for (AbstractBounds<T> b : bounds) + { + if (strictlyWrapsAround(b.left, b.right)) + return false; + } + return true; + } + @Override public int hashCode() { http://git-wip-us.apache.org/repos/asf/cassandra/blob/3aa7308e/src/java/org/apache/cassandra/dht/Bounds.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/dht/Bounds.java b/src/java/org/apache/cassandra/dht/Bounds.java index 4a5a701..9060bcf 100644 --- a/src/java/org/apache/cassandra/dht/Bounds.java +++ b/src/java/org/apache/cassandra/dht/Bounds.java @@ -32,7 +32,7 @@ public class Bounds<T extends RingPosition<T>> extends AbstractBounds<T> { super(left, right); // unlike a Range, a Bounds may not wrap - assert left.compareTo(right) <= 0 || right.isMinimum() : "[" + left + "," + right + "]"; + assert !strictlyWrapsAround(left, right) : "[" + left + "," + right + "]"; } public boolean contains(T position) http://git-wip-us.apache.org/repos/asf/cassandra/blob/3aa7308e/src/java/org/apache/cassandra/dht/ExcludingBounds.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/dht/ExcludingBounds.java b/src/java/org/apache/cassandra/dht/ExcludingBounds.java index 0d37e5c..7319356 100644 --- a/src/java/org/apache/cassandra/dht/ExcludingBounds.java +++ b/src/java/org/apache/cassandra/dht/ExcludingBounds.java @@ -31,7 +31,7 @@ public class ExcludingBounds<T extends RingPosition<T>> extends AbstractBounds<T { super(left, right); // unlike a Range, an ExcludingBounds may not wrap, nor be empty - assert left.compareTo(right) < 0 || right.isMinimum() : "(" + left + "," + right + ")"; + assert !strictlyWrapsAround(left, right) && (right.isMinimum() || left.compareTo(right) != 0) : "(" + left + "," + right + ")"; } public boolean contains(T position) http://git-wip-us.apache.org/repos/asf/cassandra/blob/3aa7308e/src/java/org/apache/cassandra/dht/IncludingExcludingBounds.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/dht/IncludingExcludingBounds.java b/src/java/org/apache/cassandra/dht/IncludingExcludingBounds.java index e9e8e8e..abcf87b 100644 --- a/src/java/org/apache/cassandra/dht/IncludingExcludingBounds.java +++ b/src/java/org/apache/cassandra/dht/IncludingExcludingBounds.java @@ -32,7 +32,7 @@ public class IncludingExcludingBounds<T extends RingPosition<T>> extends Abstrac super(left, right); // unlike a Range, an IncludingExcludingBounds may not wrap, nor have // right == left unless the right is the min token - assert left.compareTo(right) < 0 || right.isMinimum() : "[" + left + "," + right + ")"; + assert !strictlyWrapsAround(left, right) && (right.isMinimum() || left.compareTo(right) != 0) : "(" + left + "," + right + ")"; } public boolean contains(T position) http://git-wip-us.apache.org/repos/asf/cassandra/blob/3aa7308e/src/java/org/apache/cassandra/streaming/StreamSession.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/streaming/StreamSession.java b/src/java/org/apache/cassandra/streaming/StreamSession.java index 366bc33..55d7e68 100644 --- a/src/java/org/apache/cassandra/streaming/StreamSession.java +++ b/src/java/org/apache/cassandra/streaming/StreamSession.java @@ -24,8 +24,6 @@ import java.util.*; import java.util.concurrent.*; import java.util.concurrent.atomic.AtomicBoolean; -import javax.annotation.Nullable; - import com.google.common.base.Function; import com.google.common.collect.*; @@ -38,7 +36,6 @@ import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.db.ColumnFamilyStore; import org.apache.cassandra.db.Keyspace; import org.apache.cassandra.db.RowPosition; -import org.apache.cassandra.dht.AbstractBounds; import org.apache.cassandra.dht.Range; import org.apache.cassandra.dht.Token; import org.apache.cassandra.gms.*; @@ -316,9 +313,9 @@ public class StreamSession implements IEndpointStateChangeSubscriber { for (ColumnFamilyStore cfStore : stores) { - final List<AbstractBounds<RowPosition>> rowBoundsList = new ArrayList<>(ranges.size()); + final List<Range<RowPosition>> keyRanges = new ArrayList<>(ranges.size()); for (Range<Token> range : ranges) - rowBoundsList.add(Range.makeRowRange(range)); + keyRanges.add(Range.makeRowRange(range)); refs.addAll(cfStore.selectAndReference(new Function<View, List<SSTableReader>>() { public List<SSTableReader> apply(View view) @@ -328,11 +325,17 @@ public class StreamSession implements IEndpointStateChangeSubscriber permittedInstances.put(reader, reader); Set<SSTableReader> sstables = Sets.newHashSet(); - for (AbstractBounds<RowPosition> rowBounds : rowBoundsList) + for (Range<RowPosition> keyRange : keyRanges) { - // sstableInBounds may contain early opened sstables - for (SSTableReader sstable : view.sstablesInBounds(rowBounds)) + // keyRange excludes its start, while sstableInBounds is inclusive (of both start and end). + // This is fine however, because keyRange has been created from a token range through Range.makeRowRange (see above). + // And that later method uses the Token.maxKeyBound() method to creates the range, which return a "fake" key that + // sort after all keys having the token. That "fake" key cannot however be equal to any real key, so that even + // including keyRange.left will still exclude any key having the token of the original token range, and so we're + // still actually selecting what we wanted. + for (SSTableReader sstable : view.sstablesInBounds(keyRange.left, keyRange.right)) { + // sstableInBounds may contain early opened sstables if (isIncremental && sstable.isRepaired()) continue; sstable = permittedInstances.get(sstable); http://git-wip-us.apache.org/repos/asf/cassandra/blob/3aa7308e/test/unit/org/apache/cassandra/db/lifecycle/ViewTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/db/lifecycle/ViewTest.java b/test/unit/org/apache/cassandra/db/lifecycle/ViewTest.java index 4c8006a..32a81e2 100644 --- a/test/unit/org/apache/cassandra/db/lifecycle/ViewTest.java +++ b/test/unit/org/apache/cassandra/db/lifecycle/ViewTest.java @@ -61,13 +61,14 @@ public class ViewTest { RowPosition min = MockSchema.readerBounds(i); RowPosition max = MockSchema.readerBounds(j); - for (boolean minInc : new boolean[] { true, false} ) + for (boolean minInc : new boolean[] { true })//, false} ) { - for (boolean maxInc : new boolean[] { true, false} ) + for (boolean maxInc : new boolean[] { true })//, false} ) { if (i == j && !(minInc && maxInc)) continue; - List<SSTableReader> r = initialView.sstablesInBounds(AbstractBounds.bounds(min, minInc, max, maxInc)); + AbstractBounds<RowPosition> bounds = AbstractBounds.bounds(min, minInc, max, maxInc); + List<SSTableReader> r = initialView.sstablesInBounds(bounds.left, bounds.right); Assert.assertEquals(String.format("%d(%s) %d(%s)", i, minInc, j, maxInc), j - i + (minInc ? 0 : -1) + (maxInc ? 1 : 0), r.size()); } }
