Repository: cassandra Updated Branches: refs/heads/trunk 805a4aeeb -> 704469d95
Don't insert tombstones that hide indexed values into 2i patch by Sam Tunnicliffe; reviewed by Aleksey Yeschenko for CASSANDRA-7268 Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/febf3854 Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/febf3854 Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/febf3854 Branch: refs/heads/trunk Commit: febf3854bfa507c092ad5d35e3fe2d536ca78ce1 Parents: 593bba9 Author: Sam Tunnicliffe <s...@beobal.com> Authored: Tue Jun 17 16:25:29 2014 -0700 Committer: Aleksey Yeschenko <alek...@apache.org> Committed: Tue Jun 17 16:26:43 2014 -0700 ---------------------------------------------------------------------- CHANGES.txt | 4 +- .../db/index/SecondaryIndexManager.java | 26 ++++++++- .../cassandra/db/ColumnFamilyStoreTest.java | 55 ++++++++++++++++++++ 3 files changed, 82 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/febf3854/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index 28b5f29..18929d5 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,6 +1,8 @@ 1.2.17 + * Don't insert tombstones that hide indexed values into 2i (CASSANDRA-7268) * Track metrics at a keyspace level (CASSANDRA-6539) - * Add replace_address_first_boot flag to only replace if not bootstrapped (CASSANDRA-7356) + * Add replace_address_first_boot flag to only replace if not bootstrapped + (CASSANDRA-7356) * Enable keepalive for native protocol (CASSANDRA-7380) * Check internal addresses for seeds (CASSANDRA-6523) * Fix potential / by 0 in HHOM page size calculation (CASSANDRA-7354) http://git-wip-us.apache.org/repos/asf/cassandra/blob/febf3854/src/java/org/apache/cassandra/db/index/SecondaryIndexManager.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/index/SecondaryIndexManager.java b/src/java/org/apache/cassandra/db/index/SecondaryIndexManager.java index c4e4129..7fefa13 100644 --- a/src/java/org/apache/cassandra/db/index/SecondaryIndexManager.java +++ b/src/java/org/apache/cassandra/db/index/SecondaryIndexManager.java @@ -29,7 +29,6 @@ import org.apache.cassandra.config.ColumnDefinition; import org.apache.cassandra.db.*; import org.apache.cassandra.db.compaction.CompactionManager; import org.apache.cassandra.db.filter.IDiskAtomFilter; -import org.apache.cassandra.db.marshal.AbstractType; import org.apache.cassandra.dht.AbstractBounds; import org.apache.cassandra.exceptions.ConfigurationException; import org.apache.cassandra.io.sstable.ReducingKeyIterator; @@ -650,7 +649,14 @@ public class SecondaryIndexManager // where the row is invisible to both queries (the opposite seems preferable); see CASSANDRA-5540 if (!column.isMarkedForDelete()) ((PerColumnSecondaryIndex) index).insert(key.key, column); - ((PerColumnSecondaryIndex) index).delete(key.key, oldColumn); + + // Usually we want to delete the old value from the index, except when + // name/value/timestamp are all equal, but the columns themselves + // are not (as is the case when overwriting expiring columns with + // identical values and ttl) Then, we don't want to delete as the + // tombstone will hide the new value we just inserted; see CASSANDRA-7268 + if (shouldCleanupOldValue(oldColumn, column)) + ((PerColumnSecondaryIndex) index).delete(key.key, oldColumn); } } @@ -672,5 +678,21 @@ public class SecondaryIndexManager for (SecondaryIndex index : rowLevelIndexMap.values()) ((PerRowSecondaryIndex) index).index(key.key, cf); } + + private boolean shouldCleanupOldValue(IColumn oldColumn, IColumn newColumn) + { + // If any one of name/value/timestamp are different, then we + // should delete from the index. If not, then we can infer that + // at least one of the columns is an ExpiringColumn and that the + // difference is in the expiry time. In this case, we don't want to + // delete the old value from the index as the tombstone we insert + // will just hide the inserted value. + // Completely identical columns (including expiring columns with + // identical ttl & localExpirationTime) will not get this far due + // to the oldColumn.equals(newColumn) in StandardUpdater.update + return !oldColumn.name().equals(newColumn.name()) + || !oldColumn.value().equals(newColumn.value()) + || oldColumn.timestamp() != newColumn.timestamp(); + } } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/febf3854/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java b/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java index cd30297..e5354ed 100644 --- a/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java +++ b/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java @@ -25,6 +25,7 @@ import java.nio.charset.CharacterCodingException; import java.util.*; import java.util.concurrent.ExecutionException; import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; import com.google.common.base.Function; import com.google.common.collect.Iterables; @@ -418,6 +419,60 @@ public class ColumnFamilyStoreTest extends SchemaLoader } @Test + public void testIndexUpdateOverwritingExpiringColumns() throws Exception + { + // see CASSANDRA-7268 + Table table = Table.open("Keyspace2"); + + // create a row and update the birthdate value with an expiring column + RowMutation rm; + rm = new RowMutation("Keyspace2", ByteBufferUtil.bytes("k100")); + rm.add(new QueryPath("Indexed1", null, ByteBufferUtil.bytes("birthdate")), ByteBufferUtil.bytes(100L), 1, 1000); + rm.apply(); + + IndexExpression expr = new IndexExpression(ByteBufferUtil.bytes("birthdate"), IndexOperator.EQ, ByteBufferUtil.bytes(100L)); + List<IndexExpression> clause = Arrays.asList(expr); + IDiskAtomFilter filter = new IdentityQueryFilter(); + Range<RowPosition> range = Util.range("", ""); + List<Row> rows = table.getColumnFamilyStore("Indexed1").search(clause, range, 100, filter); + assertEquals(1,rows.size()); + + // requires a 1s sleep because we calculate local expiry time as (now() / 1000) + ttl + TimeUnit.SECONDS.sleep(1); + + // now overwrite with the same name/value/ttl, but the local expiry time will be different + rm = new RowMutation("Keyspace2", ByteBufferUtil.bytes("k100")); + rm.add(new QueryPath("Indexed1", null, ByteBufferUtil.bytes("birthdate")), ByteBufferUtil.bytes(100L), 1, 1000); + rm.apply(); + + rows = table.getColumnFamilyStore("Indexed1").search(clause, range, 100, filter); + assertEquals(1,rows.size()); + + // check that modifying the indexed value using the same timestamp behaves as expected + rm = new RowMutation("Keyspace2", ByteBufferUtil.bytes("k101")); + rm.add(new QueryPath("Indexed1", null, ByteBufferUtil.bytes("birthdate")), ByteBufferUtil.bytes(101L), 1, 1000); + rm.apply(); + + expr = new IndexExpression(ByteBufferUtil.bytes("birthdate"), IndexOperator.EQ, ByteBufferUtil.bytes(101L)); + clause = Arrays.asList(expr); + rows = table.getColumnFamilyStore("Indexed1").search(clause, range, 100, filter); + assertEquals(1,rows.size()); + + TimeUnit.SECONDS.sleep(1); + rm = new RowMutation("Keyspace2", ByteBufferUtil.bytes("k101")); + rm.add(new QueryPath("Indexed1", null, ByteBufferUtil.bytes("birthdate")), ByteBufferUtil.bytes(102L), 1, 1000); + rm.apply(); + // search for the old value + rows = table.getColumnFamilyStore("Indexed1").search(clause, range, 100, filter); + assertEquals(0,rows.size()); + // and for the new + expr = new IndexExpression(ByteBufferUtil.bytes("birthdate"), IndexOperator.EQ, ByteBufferUtil.bytes(102L)); + clause = Arrays.asList(expr); + rows = table.getColumnFamilyStore("Indexed1").search(clause, range, 100, filter); + assertEquals(1,rows.size()); + } + + @Test public void testDeleteOfInconsistentValuesInKeysIndex() throws Exception { String keySpace = "Keyspace2";
