diff --git a/CHANGES.txt b/CHANGES.txt index 3a1b4c2de7..e9f34ed331 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -23,6 +23,7 @@ * (cql3) fix range queries containing unqueried results (CASSANDRA-4372) * (cql3) allow updating column_alias types (CASSANDRA-4041) * (cql3) Fix deletion bug (CASSANDRA-4193) + * Fix computation of overlapping sstable for leveled compaction (CASSANDRA-4321) Merged from 1.0: * Set gc_grace on index CF to 0 (CASSANDRA-4314) diff --git a/src/java/org/apache/cassandra/db/compaction/LeveledManifest.java b/src/java/org/apache/cassandra/db/compaction/LeveledManifest.java index dca8b7d782..beb7d74940 100644 --- a/src/java/org/apache/cassandra/db/compaction/LeveledManifest.java +++ b/src/java/org/apache/cassandra/db/compaction/LeveledManifest.java @@ -27,13 +27,14 @@ import java.io.IOException; import java.util.*; import com.google.common.collect.Iterables; +import com.google.common.collect.Sets; import com.google.common.primitives.Ints; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.apache.cassandra.db.ColumnFamilyStore; import org.apache.cassandra.db.RowPosition; -import org.apache.cassandra.dht.Range; +import org.apache.cassandra.dht.Bounds; import org.apache.cassandra.dht.Token; import org.apache.cassandra.io.sstable.SSTable; import org.apache.cassandra.io.sstable.SSTableReader; @@ -287,24 +288,6 @@ public class LeveledManifest return Collections.emptyList(); } - /** - * Go through candidates collection and check if any of the SSTables are marked as suspected. - * - * @param candidates The SSTable collection to examine. - * - * @return true if collection has at least one SSTable marked as suspected, false otherwise. - */ - private boolean hasSuspectSSTables(Collection candidates) - { - for (SSTableReader candidate : candidates) - { - if (candidate.isMarkedSuspect()) - return true; - } - - return false; - } - public int getLevelSize(int i) { return generations.length > i ? generations[i].size() : 0; @@ -350,16 +333,50 @@ public class LeveledManifest sstableGenerations.put(sstable, Integer.valueOf(level)); } - private static Set overlapping(SSTableReader sstable, Iterable candidates) + private static Set overlapping(Collection candidates, Iterable others) { - Set overlapped = new HashSet(); - overlapped.add(sstable); - - Range promotedRange = new Range(sstable.first.token, sstable.last.token); - for (SSTableReader candidate : candidates) + assert !candidates.isEmpty(); + /* + * Picking each sstable from others that overlap one of the sstable of candidates is not enough + * because you could have the following situation: + * candidates = [ s1(a, c), s2(m, z) ] + * others = [ s3(e, g) ] + * In that case, s2 overlaps none of s1 or s2, but if we compact s1 with s2, the resulting sstable will + * overlap s3, so we must return s3. + * + * Thus, the correct approach is to pick sstables overlapping anything between the first key in all + * the candidate sstables, and the last. + */ + Iterator iter = candidates.iterator(); + SSTableReader sstable = iter.next(); + Token first = sstable.first.token; + Token last = sstable.last.token; + while (iter.hasNext()) { - Range candidateRange = new Range(candidate.first.token, candidate.last.token); - if (candidateRange.intersects(promotedRange)) + sstable = iter.next(); + first = first.compareTo(sstable.first.token) <= 0 ? first : sstable.first.token; + last = last.compareTo(sstable.last.token) >= 0 ? last : sstable.last.token; + } + return overlapping(first, last, others); + } + + private static Set overlapping(SSTableReader sstable, Iterable others) + { + return overlapping(sstable.first.token, sstable.last.token, others); + } + + /** + * @return sstables from @param sstables that contain keys between @param start and @param end, inclusive. + */ + private static Set overlapping(Token start, Token end, Iterable sstables) + { + assert start.compareTo(end) <= 0; + Set overlapped = new HashSet(); + Bounds promotedBounds = new Bounds(start, end); + for (SSTableReader candidate : sstables) + { + Bounds candidateBounds = new Bounds(candidate.first.token, candidate.last.token); + if (candidateBounds.intersects(promotedBounds)) overlapped.add(candidate); } return overlapped; @@ -394,7 +411,7 @@ public class LeveledManifest if (candidates.contains(sstable)) continue; - for (SSTableReader newCandidate : overlapping(sstable, remaining)) + for (SSTableReader newCandidate : Sets.union(Collections.singleton(sstable), overlapping(sstable, remaining))) { if (!newCandidate.isMarkedSuspect()) { @@ -412,8 +429,7 @@ public class LeveledManifest if (SSTable.getTotalBytes(candidates) > maxSSTableSizeInBytes) { // add sstables from L1 that overlap candidates - for (SSTableReader candidate : new ArrayList(candidates)) - candidates.addAll(overlapping(candidate, generations[1])); + candidates.addAll(overlapping(candidates, generations[1])); } return candidates; } @@ -421,8 +437,7 @@ public class LeveledManifest if (SSTable.getTotalBytes(candidates) > maxSSTableSizeInBytes) { // add sstables from L1 that overlap candidates - for (SSTableReader candidate : new ArrayList(candidates)) - candidates.addAll(overlapping(candidate, generations[1])); + candidates.addAll(overlapping(candidates, generations[1])); break; } } @@ -450,7 +465,7 @@ public class LeveledManifest while (true) { SSTableReader sstable = generations[level].get(i); - Set candidates = overlapping(sstable, generations[(level + 1)]); + Set candidates = Sets.union(Collections.singleton(sstable), overlapping(sstable, generations[(level + 1)])); for (SSTableReader candidate : candidates) { if (candidate.isMarkedSuspect()) diff --git a/src/java/org/apache/cassandra/dht/Bounds.java b/src/java/org/apache/cassandra/dht/Bounds.java index 9ff830ee23..0e4dbdf7f2 100644 --- a/src/java/org/apache/cassandra/dht/Bounds.java +++ b/src/java/org/apache/cassandra/dht/Bounds.java @@ -62,6 +62,12 @@ public class Bounds extends AbstractBounds return new Pair, AbstractBounds>(lb, rb); } + public boolean intersects(Bounds that) + { + // We either contains one of the that bounds, or we are fully contained into that. + return contains(that.left) || contains(that.right) || that.contains(left); + } + public List> unwrap() { // Bounds objects never wrap diff --git a/src/java/org/apache/cassandra/io/sstable/SSTableWriter.java b/src/java/org/apache/cassandra/io/sstable/SSTableWriter.java index 3e7e7a0f73..5619951361 100644 --- a/src/java/org/apache/cassandra/io/sstable/SSTableWriter.java +++ b/src/java/org/apache/cassandra/io/sstable/SSTableWriter.java @@ -130,8 +130,8 @@ public class SSTableWriter extends SSTable private long beforeAppend(DecoratedKey decoratedKey) throws IOException { assert decoratedKey != null : "Keys must not be null"; - assert lastWrittenKey == null || lastWrittenKey.compareTo(decoratedKey) < 0 - : "Last written key " + lastWrittenKey + " >= current key " + decoratedKey + " writing into " + getFilename(); + if (lastWrittenKey != null && lastWrittenKey.compareTo(decoratedKey) >= 0) + throw new RuntimeException("Last written key " + lastWrittenKey + " >= current key " + decoratedKey + " writing into " + getFilename()); return (lastWrittenKey == null) ? 0 : dataFile.getFilePointer(); }