diff --git a/CHANGES.txt b/CHANGES.txt index aa5f23510b..8794509da3 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -19,6 +19,7 @@ * Fix anticompaction blocking ANTI_ENTROPY stage (CASSANDRA-9151) * Repair waits for anticompaction to finish (CASSANDRA-9097) Merged from 2.0: + * Fix counting of tombstones for TombstoneOverwhelmingException (CASSANDRA-9299) * Fix ReconnectableSnitch reconnecting to peers during upgrade (CASSANDRA-6702) * Include keyspace and table name in error log for collections over the size limit (CASSANDRA-9286) diff --git a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java index 4438afd83b..41ceb5030c 100644 --- a/src/java/org/apache/cassandra/db/ColumnFamilyStore.java +++ b/src/java/org/apache/cassandra/db/ColumnFamilyStore.java @@ -1768,7 +1768,7 @@ public class ColumnFamilyStore implements ColumnFamilyStoreMBean if (filter.filter instanceof SliceQueryFilter) { // Log the number of tombstones scanned on single key queries - metric.tombstoneScannedHistogram.update(((SliceQueryFilter) filter.filter).lastIgnored()); + metric.tombstoneScannedHistogram.update(((SliceQueryFilter) filter.filter).lastTombstones()); metric.liveScannedHistogram.update(((SliceQueryFilter) filter.filter).lastLive()); } } diff --git a/src/java/org/apache/cassandra/db/filter/ColumnCounter.java b/src/java/org/apache/cassandra/db/filter/ColumnCounter.java index 86cfc4077e..8be26e10a4 100644 --- a/src/java/org/apache/cassandra/db/filter/ColumnCounter.java +++ b/src/java/org/apache/cassandra/db/filter/ColumnCounter.java @@ -29,7 +29,7 @@ import org.apache.cassandra.db.DeletionInfo; public class ColumnCounter { protected int live; - protected int ignored; + protected int tombstones; protected final long timestamp; public ColumnCounter(long timestamp) @@ -39,15 +39,15 @@ public class ColumnCounter public void count(Cell cell, DeletionInfo.InOrderTester tester) { - if (!isLive(cell, tester, timestamp)) - ignored++; - else - live++; - } + // The cell is shadowed by a higher-level deletion, and won't be retained. + // For the purposes of this counter, we don't care if it's a tombstone or not. + if (tester.isDeleted(cell)) + return; - protected static boolean isLive(Cell cell, DeletionInfo.InOrderTester tester, long timestamp) - { - return cell.isLive(timestamp) && !tester.isDeleted(cell); + if (cell.isLive(timestamp)) + live++; + else + tombstones++; } public int live() @@ -55,9 +55,9 @@ public class ColumnCounter return live; } - public int ignored() + public int tombstones() { - return ignored; + return tombstones; } public ColumnCounter countAll(ColumnFamily container) @@ -98,9 +98,12 @@ public class ColumnCounter public void count(Cell cell, DeletionInfo.InOrderTester tester) { - if (!isLive(cell, tester, timestamp)) + if (tester.isDeleted(cell)) + return; + + if (!cell.isLive(timestamp)) { - ignored++; + tombstones++; return; } diff --git a/src/java/org/apache/cassandra/db/filter/SliceQueryFilter.java b/src/java/org/apache/cassandra/db/filter/SliceQueryFilter.java index 38947bf550..1195d4caaa 100644 --- a/src/java/org/apache/cassandra/db/filter/SliceQueryFilter.java +++ b/src/java/org/apache/cassandra/db/filter/SliceQueryFilter.java @@ -203,28 +203,35 @@ public class SliceQueryFilter implements IDiskAtomFilter while (reducedColumns.hasNext()) { Cell cell = reducedColumns.next(); + if (logger.isTraceEnabled()) - logger.trace(String.format("collecting %s of %s: %s", - columnCounter.live(), count, cell.getString(container.getComparator()))); + logger.trace("collecting {} of {}: {}", columnCounter.live(), count, cell.getString(container.getComparator())); + + // An expired tombstone will be immediately discarded in memory, and needn't be counted. + if (cell.getLocalDeletionTime() < gcBefore) + continue; columnCounter.count(cell, tester); if (columnCounter.live() > count) break; - if (respectTombstoneThresholds() && columnCounter.ignored() > DatabaseDescriptor.getTombstoneFailureThreshold()) + if (respectTombstoneThresholds() && columnCounter.tombstones() > DatabaseDescriptor.getTombstoneFailureThreshold()) { - Tracing.trace("Scanned over {} tombstones; query aborted (see tombstone_failure_threshold)", DatabaseDescriptor.getTombstoneFailureThreshold()); + Tracing.trace("Scanned over {} tombstones; query aborted (see tombstone_failure_threshold)", + DatabaseDescriptor.getTombstoneFailureThreshold()); logger.error("Scanned over {} tombstones in {}.{}; query aborted (see tombstone_failure_threshold)", - DatabaseDescriptor.getTombstoneFailureThreshold(), container.metadata().ksName, container.metadata().cfName); + DatabaseDescriptor.getTombstoneFailureThreshold(), + container.metadata().ksName, + container.metadata().cfName); throw new TombstoneOverwhelmingException(); } container.maybeAppendColumn(cell, tester, gcBefore); } - Tracing.trace("Read {} live and {} tombstoned cells", columnCounter.live(), columnCounter.ignored()); - if (logger.isWarnEnabled() && respectTombstoneThresholds() && columnCounter.ignored() > DatabaseDescriptor.getTombstoneWarnThreshold()) + Tracing.trace("Read {} live and {} tombstone cells", columnCounter.live(), columnCounter.tombstones()); + if (logger.isWarnEnabled() && respectTombstoneThresholds() && columnCounter.tombstones() > DatabaseDescriptor.getTombstoneWarnThreshold()) { StringBuilder sb = new StringBuilder(); CellNameType type = container.metadata().comparator; @@ -240,9 +247,9 @@ public class SliceQueryFilter implements IDiskAtomFilter sb.append(']'); } - String msg = String.format("Read %d live and %d tombstoned cells in %s.%s for key: %1.512s (see tombstone_warn_threshold). %d columns were requested, slices=%1.512s", + String msg = String.format("Read %d live and %d tombstone cells in %s.%s for key: %1.512s (see tombstone_warn_threshold). %d columns were requested, slices=%1.512s", columnCounter.live(), - columnCounter.ignored(), + columnCounter.tombstones(), container.metadata().ksName, container.metadata().cfName, container.metadata().getKeyValidator().getString(key.getKey()), @@ -324,9 +331,9 @@ public class SliceQueryFilter implements IDiskAtomFilter return columnCounter == null ? 0 : Math.min(columnCounter.live(), count); } - public int lastIgnored() + public int lastTombstones() { - return columnCounter == null ? 0 : columnCounter.ignored(); + return columnCounter == null ? 0 : columnCounter.tombstones(); } public int lastLive() @@ -438,8 +445,7 @@ public class SliceQueryFilter implements IDiskAtomFilter slices[i] = type.sliceSerializer().deserialize(in, version); boolean reversed = in.readBoolean(); int count = in.readInt(); - int compositesToGroup = -1; - compositesToGroup = in.readInt(); + int compositesToGroup = in.readInt(); return new SliceQueryFilter(slices, reversed, count, compositesToGroup); } @@ -464,7 +470,7 @@ public class SliceQueryFilter implements IDiskAtomFilter { final DeletionInfo delInfo = source.deletionInfo(); if (!delInfo.hasRanges() || slices.length == 0) - return Iterators.emptyIterator(); + return Iterators.emptyIterator(); return new AbstractIterator() { diff --git a/test/unit/org/apache/cassandra/cql3/SliceQueryFilterWithTombstonesTest.java b/test/unit/org/apache/cassandra/cql3/SliceQueryFilterWithTombstonesTest.java new file mode 100644 index 0000000000..0cb98192f1 --- /dev/null +++ b/test/unit/org/apache/cassandra/cql3/SliceQueryFilterWithTombstonesTest.java @@ -0,0 +1,166 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.cassandra.cql3; + +import java.util.concurrent.TimeUnit; + +import org.junit.AfterClass; +import org.junit.BeforeClass; +import org.junit.Test; + +import org.apache.cassandra.config.DatabaseDescriptor; +import org.apache.cassandra.db.filter.TombstoneOverwhelmingException; + +import static junit.framework.Assert.assertTrue; +import static junit.framework.Assert.fail; + +/** + * Test that TombstoneOverwhelmingException gets thrown when it should be and doesn't when it shouldn't be. + */ +public class SliceQueryFilterWithTombstonesTest extends CQLTester +{ + static final int ORIGINAL_THRESHOLD = DatabaseDescriptor.getTombstoneFailureThreshold(); + static final int THRESHOLD = 100; + + @BeforeClass + public static void setUp() throws Throwable + { + DatabaseDescriptor.setTombstoneFailureThreshold(THRESHOLD); + } + + @AfterClass + public static void tearDown() + { + DatabaseDescriptor.setTombstoneFailureThreshold(ORIGINAL_THRESHOLD); + } + + @Test + public void testBelowThresholdSelect() throws Throwable + { + createTable("CREATE TABLE %s (a text, b text, c text, PRIMARY KEY (a, b));"); + + // insert exactly the amount of tombstones that shouldn't trigger an exception + for (int i = 0; i < THRESHOLD; i++) + execute("INSERT INTO %s (a, b, c) VALUES ('key', 'column" + i + "', null);"); + + try + { + execute("SELECT * FROM %s WHERE a = 'key';"); + } + catch (Throwable e) + { + fail("SELECT with tombstones below the threshold should not have failed, but has: " + e); + } + } + + @Test + public void testBeyondThresholdSelect() throws Throwable + { + createTable("CREATE TABLE %s (a text, b text, c text, PRIMARY KEY (a, b));"); + + // insert exactly the amount of tombstones that *SHOULD* trigger an exception + for (int i = 0; i < THRESHOLD + 1; i++) + execute("INSERT INTO %s (a, b, c) VALUES ('key', 'column" + i + "', null);"); + + try + { + execute("SELECT * FROM %s WHERE a = 'key';"); + fail("SELECT with tombstones beyond the threshold should have failed, but hasn't"); + } + catch (Throwable e) + { + assertTrue(e instanceof TombstoneOverwhelmingException); + } + } + + @Test + public void testAllShadowedSelect() throws Throwable + { + createTable("CREATE TABLE %s (a text, b text, c text, PRIMARY KEY (a, b));"); + + // insert exactly the amount of tombstones that *SHOULD* normally trigger an exception + for (int i = 0; i < THRESHOLD + 1; i++) + execute("INSERT INTO %s (a, b, c) VALUES ('key', 'column" + i + "', null);"); + + // delete all with a partition level tombstone + execute("DELETE FROM %s WHERE a = 'key'"); + + try + { + execute("SELECT * FROM %s WHERE a = 'key';"); + } + catch (Throwable e) + { + fail("SELECT with tombstones shadowed by a partition tombstone should not have failed, but has: " + e); + } + } + + @Test + public void testLiveShadowedCellsSelect() throws Throwable + { + createTable("CREATE TABLE %s (a text, b text, c text, PRIMARY KEY (a, b));"); + + for (int i = 0; i < THRESHOLD + 1; i++) + execute("INSERT INTO %s (a, b, c) VALUES ('key', 'column" + i + "', 'column');"); + + // delete all with a partition level tombstone + execute("DELETE FROM %s WHERE a = 'key'"); + + try + { + execute("SELECT * FROM %s WHERE a = 'key';"); + } + catch (Throwable e) + { + fail("SELECT with regular cells shadowed by a partition tombstone should not have failed, but has: " + e); + } + } + + @Test + public void testExpiredTombstones() throws Throwable + { + createTable("CREATE TABLE %s (a text, b text, c text, PRIMARY KEY (a, b)) WITH gc_grace_seconds = 1;"); + + for (int i = 0; i < THRESHOLD + 1; i++) + execute("INSERT INTO %s (a, b, c) VALUES ('key', 'column" + i + "', null);"); + + // not yet past gc grace - must throw a TOE + try + { + execute("SELECT * FROM %s WHERE a = 'key';"); + fail("SELECT with tombstones beyond the threshold should have failed, but hasn't"); + } + catch (Throwable e) + { + assertTrue(e instanceof TombstoneOverwhelmingException); + } + + // sleep past gc grace + TimeUnit.SECONDS.sleep(2); + + // past gc grace - must not throw a TOE now + try + { + execute("SELECT * FROM %s WHERE a = 'key';"); + } + catch (Throwable e) + { + fail("SELECT with expired tombstones beyond the threshold should not have failed, but has: " + e); + } + } +}