From 3aa7308e8f86969158c8d919c3f77658ae7c4fc3 Mon Sep 17 00:00:00 2001 From: Sylvain Lebresne Date: Wed, 12 Aug 2015 16:34:07 -0400 Subject: [PATCH] Improve assertions around some of the usage of AbstractBounds patch by Sylvain Lebresne and Ariel Weisberg; reviewed by Aleksey Yeschenko for CASSANDRA-9462 --- .../cassandra/db/ColumnFamilyStore.java | 18 ++++++++++--- .../cassandra/db/SizeEstimatesRecorder.java | 3 ++- .../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 +- .../dht/IncludingExcludingBounds.java | 2 +- .../cassandra/streaming/StreamSession.java | 19 ++++++++------ .../cassandra/db/lifecycle/ViewTest.java | 7 +++--- 9 files changed, 70 insertions(+), 21 deletions(-) diff --git a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java index 482e3eebf2..8bda6b25f6 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> viewFilter(final AbstractBounds rowBounds) { + assert !AbstractBounds.strictlyWrapsAround(rowBounds.left, rowBounds.right); return new Function>() { public List 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> viewFilter(final Collection> rowBoundsCollection, final boolean includeRepaired) { + assert AbstractBounds.noneStrictlyWrapsAround(rowBoundsCollection); return new Function>() { public List apply(View view) @@ -1951,7 +1958,12 @@ public class ColumnFamilyStore implements ColumnFamilyStoreMBean Set sstables = Sets.newHashSet(); for (AbstractBounds 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(); diff --git a/src/java/org/apache/cassandra/db/SizeEstimatesRecorder.java b/src/java/org/apache/cassandra/db/SizeEstimatesRecorder.java index c68109ce11..f054315019 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> localRanges) { + List> unwrappedRanges = Range.normalize(localRanges); // for each local primary range, estimate (crudely) mean partition size and partitions count. Map, Pair> estimates = new HashMap<>(localRanges.size()); - for (Range range : localRanges) + for (Range range : unwrappedRanges) { // filter sstables that have partitions in this range. Refs refs = null; diff --git a/src/java/org/apache/cassandra/db/lifecycle/View.java b/src/java/org/apache/cassandra/db/lifecycle/View.java index 0d1100bfbc..73ba131e84 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 sstablesInBounds(AbstractBounds 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 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.create(rowBounds.left, stopInTree)); + + RowPosition stopInTree = right.isMinimum() ? intervalTree.max() : right; + return intervalTree.search(Interval.create(left, stopInTree)); } // METHODS TO CONSTRUCT FUNCTIONS FOR MODIFYING A VIEW: diff --git a/src/java/org/apache/cassandra/dht/AbstractBounds.java b/src/java/org/apache/cassandra/dht/AbstractBounds.java index b5ffc22929..c33ffc088d 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> 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. + *

+ * 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 > boolean strictlyWrapsAround(T left, T right) + { + return !(left.compareTo(right) <= 0 || right.isMinimum()); + } + + public static > boolean noneStrictlyWrapsAround(Collection> bounds) + { + for (AbstractBounds b : bounds) + { + if (strictlyWrapsAround(b.left, b.right)) + return false; + } + return true; + } + @Override public int hashCode() { diff --git a/src/java/org/apache/cassandra/dht/Bounds.java b/src/java/org/apache/cassandra/dht/Bounds.java index 4a5a7014ca..9060bcfd59 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> extends AbstractBounds { 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) diff --git a/src/java/org/apache/cassandra/dht/ExcludingBounds.java b/src/java/org/apache/cassandra/dht/ExcludingBounds.java index 0d37e5c099..7319356afc 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> extends AbstractBounds> 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) diff --git a/src/java/org/apache/cassandra/streaming/StreamSession.java b/src/java/org/apache/cassandra/streaming/StreamSession.java index 366bc33b16..55d7e68a5d 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> rowBoundsList = new ArrayList<>(ranges.size()); + final List> keyRanges = new ArrayList<>(ranges.size()); for (Range range : ranges) - rowBoundsList.add(Range.makeRowRange(range)); + keyRanges.add(Range.makeRowRange(range)); refs.addAll(cfStore.selectAndReference(new Function>() { public List apply(View view) @@ -328,11 +325,17 @@ public class StreamSession implements IEndpointStateChangeSubscriber permittedInstances.put(reader, reader); Set sstables = Sets.newHashSet(); - for (AbstractBounds rowBounds : rowBoundsList) + for (Range 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); diff --git a/test/unit/org/apache/cassandra/db/lifecycle/ViewTest.java b/test/unit/org/apache/cassandra/db/lifecycle/ViewTest.java index 4c8006a7c8..32a81e2844 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 r = initialView.sstablesInBounds(AbstractBounds.bounds(min, minInc, max, maxInc)); + AbstractBounds bounds = AbstractBounds.bounds(min, minInc, max, maxInc); + List 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()); } }