cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From alek...@apache.org
Subject [1/4] git commit: Don't insert tombstones that hide indexed values into 2i
Date Wed, 18 Jun 2014 00:43:09 GMT
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 <sam@beobal.com>
Authored: Tue Jun 17 16:25:29 2014 -0700
Committer: Aleksey Yeschenko <aleksey@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";


Mime
View raw message