You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by al...@apache.org on 2015/08/14 20:23:02 UTC
cassandra git commit: Improve assertions around some of the usage of
AbstractBounds
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 <sy...@datastax.com>
Authored: Wed Aug 12 16:34:07 2015 -0400
Committer: Aleksey Yeschenko <al...@apache.org>
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());
}
}