diff --git a/CHANGES.txt b/CHANGES.txt index fd7c62b02d..4c9e69d9f3 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -19,8 +19,10 @@ Merged from 2.0: * Make StreamSession#closeSession() idempotent (CASSANDRA-7262) * Fix infinite loop on exception while streaming (CASSANDRA-7330) Merged from 1.2: + * 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) diff --git a/src/java/org/apache/cassandra/db/index/SecondaryIndexManager.java b/src/java/org/apache/cassandra/db/index/SecondaryIndexManager.java index 36c7e1e7f2..f78dc86c78 100644 --- a/src/java/org/apache/cassandra/db/index/SecondaryIndexManager.java +++ b/src/java/org/apache/cassandra/db/index/SecondaryIndexManager.java @@ -711,9 +711,19 @@ public class SecondaryIndexManager if (index instanceof PerColumnSecondaryIndex) { if (cell.isLive()) + { ((PerColumnSecondaryIndex) index).update(key.getKey(), oldCell, cell, opGroup); + } else - ((PerColumnSecondaryIndex) index).delete(key.getKey(), oldCell, opGroup); + { + // 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(oldCell, cell)) + ((PerColumnSecondaryIndex) index).delete(key.getKey(), oldCell, opGroup); + } } } } @@ -733,5 +743,21 @@ public class SecondaryIndexManager for (SecondaryIndex index : rowLevelIndexMap.values()) ((PerRowSecondaryIndex) index).index(key.getKey(), cf); } + + private boolean shouldCleanupOldValue(Cell oldCell, Cell newCell) + { + // 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 cells 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 cells (including expiring columns with + // identical ttl & localExpirationTime) will not get this far due + // to the oldCell.equals(newColumn) in StandardUpdater.update + return !oldCell.name().equals(newCell.name()) + || !oldCell.value().equals(newCell.value()) + || oldCell.timestamp() != newCell.timestamp(); + } } } diff --git a/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java b/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java index b178e48b58..dda9b658bd 100644 --- a/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java +++ b/test/unit/org/apache/cassandra/db/ColumnFamilyStoreTest.java @@ -38,6 +38,7 @@ import java.util.TreeSet; import java.util.UUID; 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; @@ -452,6 +453,60 @@ public class ColumnFamilyStoreTest extends SchemaLoader } + @Test + public void testIndexUpdateOverwritingExpiringColumns() throws Exception + { + // see CASSANDRA-7268 + Keyspace keyspace = Keyspace.open("Keyspace2"); + + // create a row and update the birthdate value with an expiring column + Mutation rm; + rm = new Mutation("Keyspace2", ByteBufferUtil.bytes("k100")); + rm.add("Indexed1", cellname("birthdate"), ByteBufferUtil.bytes(100L), 1, 1000); + rm.apply(); + + IndexExpression expr = new IndexExpression(ByteBufferUtil.bytes("birthdate"), IndexExpression.Operator.EQ, ByteBufferUtil.bytes(100L)); + List clause = Arrays.asList(expr); + IDiskAtomFilter filter = new IdentityQueryFilter(); + Range range = Util.range("", ""); + List rows = keyspace.getColumnFamilyStore("Indexed1").search(range, clause, filter, 100); + 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 Mutation("Keyspace2", ByteBufferUtil.bytes("k100")); + rm.add("Indexed1", cellname("birthdate"), ByteBufferUtil.bytes(100L), 1, 1000); + rm.apply(); + + rows = keyspace.getColumnFamilyStore("Indexed1").search(range, clause, filter, 100); + assertEquals(1, rows.size()); + + // check that modifying the indexed value using the same timestamp behaves as expected + rm = new Mutation("Keyspace2", ByteBufferUtil.bytes("k101")); + rm.add("Indexed1", cellname("birthdate"), ByteBufferUtil.bytes(101L), 1, 1000); + rm.apply(); + + expr = new IndexExpression(ByteBufferUtil.bytes("birthdate"), IndexExpression.Operator.EQ, ByteBufferUtil.bytes(101L)); + clause = Arrays.asList(expr); + rows = keyspace.getColumnFamilyStore("Indexed1").search(range, clause, filter, 100); + assertEquals(1, rows.size()); + + TimeUnit.SECONDS.sleep(1); + rm = new Mutation("Keyspace2", ByteBufferUtil.bytes("k101")); + rm.add("Indexed1", cellname("birthdate"), ByteBufferUtil.bytes(102L), 1, 1000); + rm.apply(); + // search for the old value + rows = keyspace.getColumnFamilyStore("Indexed1").search(range, clause, filter, 100); + assertEquals(0, rows.size()); + // and for the new + expr = new IndexExpression(ByteBufferUtil.bytes("birthdate"), IndexExpression.Operator.EQ, ByteBufferUtil.bytes(102L)); + clause = Arrays.asList(expr); + rows = keyspace.getColumnFamilyStore("Indexed1").search(range, clause, filter, 100); + assertEquals(1, rows.size()); + } + @Test public void testDeleteOfInconsistentValuesInKeysIndex() throws Exception {