cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From slebre...@apache.org
Subject [1/2] cassandra git commit: Don't try to validate values for cell tombstones
Date Mon, 02 May 2016 10:16:56 GMT
Repository: cassandra
Updated Branches:
  refs/heads/trunk 1800a5b62 -> a62f70dd1


Don't try to validate values for cell tombstones

patch by slebresne; reviewed by blerer for CASSANDRA-11618


Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo
Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/c83f20a3
Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/c83f20a3
Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/c83f20a3

Branch: refs/heads/trunk
Commit: c83f20a3e426cc2c3ee957409898f0b41f19916e
Parents: 620efdc
Author: Sylvain Lebresne <sylvain@datastax.com>
Authored: Wed Apr 20 15:57:48 2016 +0200
Committer: Sylvain Lebresne <sylvain@datastax.com>
Committed: Mon May 2 12:15:01 2016 +0200

----------------------------------------------------------------------
 CHANGES.txt                                     |  1 +
 .../apache/cassandra/db/rows/AbstractCell.java  | 15 ++--
 test/unit/org/apache/cassandra/db/CellTest.java | 77 +++++++++++++++++++-
 3 files changed, 86 insertions(+), 7 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/cassandra/blob/c83f20a3/CHANGES.txt
----------------------------------------------------------------------
diff --git a/CHANGES.txt b/CHANGES.txt
index 64bcbd8..887ed19 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -1,4 +1,5 @@
 3.0.6
+ * Potential error replaying commitlog with smallint/tinyint/date/time types (CASSANDRA-11618)
  * Improve tombstone printing in sstabledump (CASSANDRA-11655)
  * Fix paging for range queries where all clustering columns are specified (CASSANDRA-11669)
  * Don't require HEAP_NEW_SIZE to be set when using G1 (CASSANDRA-11600)

http://git-wip-us.apache.org/repos/asf/cassandra/blob/c83f20a3/src/java/org/apache/cassandra/db/rows/AbstractCell.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/rows/AbstractCell.java b/src/java/org/apache/cassandra/db/rows/AbstractCell.java
index 882c0e0..00fc286 100644
--- a/src/java/org/apache/cassandra/db/rows/AbstractCell.java
+++ b/src/java/org/apache/cassandra/db/rows/AbstractCell.java
@@ -52,8 +52,6 @@ public abstract class AbstractCell extends Cell
 
     public void validate()
     {
-        column().validateCellValue(value());
-
         if (ttl() < 0)
             throw new MarshalException("A TTL should not be negative");
         if (localDeletionTime() < 0)
@@ -61,9 +59,16 @@ public abstract class AbstractCell extends Cell
         if (isExpiring() && localDeletionTime() == NO_DELETION_TIME)
             throw new MarshalException("Shoud not have a TTL without an associated local
deletion time");
 
-        // If cell is a tombstone, it shouldn't have a value.
-        if (isTombstone() && value().hasRemaining())
-            throw new MarshalException("A tombstone should not have a value");
+        if (isTombstone())
+        {
+            // If cell is a tombstone, it shouldn't have a value.
+            if (value().hasRemaining())
+                throw new MarshalException("A tombstone should not have a value");
+        }
+        else
+        {
+            column().validateCellValue(value());
+        }
 
         if (path() != null)
             column().validateCellPath(path());

http://git-wip-us.apache.org/repos/asf/cassandra/blob/c83f20a3/test/unit/org/apache/cassandra/db/CellTest.java
----------------------------------------------------------------------
diff --git a/test/unit/org/apache/cassandra/db/CellTest.java b/test/unit/org/apache/cassandra/db/CellTest.java
index 5953255..9072f98 100644
--- a/test/unit/org/apache/cassandra/db/CellTest.java
+++ b/test/unit/org/apache/cassandra/db/CellTest.java
@@ -30,12 +30,12 @@ import org.junit.Test;
 import org.apache.cassandra.config.CFMetaData;
 import org.apache.cassandra.config.ColumnDefinition;
 import org.apache.cassandra.cql3.ColumnIdentifier;
-import org.apache.cassandra.db.marshal.IntegerType;
-import org.apache.cassandra.db.marshal.MapType;
+import org.apache.cassandra.db.marshal.*;
 import org.apache.cassandra.db.rows.*;
 import org.apache.cassandra.exceptions.ConfigurationException;
 import org.apache.cassandra.SchemaLoader;
 import org.apache.cassandra.schema.KeyspaceParams;
+import org.apache.cassandra.serializers.MarshalException;
 import org.apache.cassandra.utils.ByteBufferUtil;
 import org.apache.cassandra.utils.FBUtilities;
 
@@ -53,6 +53,8 @@ public class CellTest
                                                              .addRegularColumn("m", MapType.getInstance(IntegerType.instance,
IntegerType.instance, true))
                                                              .build();
 
+    private static final CFMetaData fakeMetadata = CFMetaData.createFake("fakeKS", "fakeTable");
+
     @BeforeClass
     public static void defineSchema() throws ConfigurationException
     {
@@ -60,6 +62,16 @@ public class CellTest
         SchemaLoader.createKeyspace(KEYSPACE1, KeyspaceParams.simple(1), cfm, cfm2);
     }
 
+    private static ColumnDefinition fakeColumn(String name, AbstractType<?> type)
+    {
+        return new ColumnDefinition(fakeMetadata.ksName,
+                                    fakeMetadata.cfName,
+                                    ColumnIdentifier.getInterned(name, false),
+                                    type,
+                                    ColumnDefinition.NO_POSITION,
+                                    ColumnDefinition.Kind.REGULAR);
+    }
+
     @Test
     public void testConflictingTypeEquality()
     {
@@ -83,6 +95,67 @@ public class CellTest
         }
     }
 
+    private void assertValid(Cell cell)
+    {
+        try
+        {
+            cell.validate();
+        }
+        catch (Exception e)
+        {
+            Assert.fail("Cell should be valid but got error: " + e);
+        }
+    }
+
+    private void assertInvalid(Cell cell)
+    {
+        try
+        {
+            cell.validate();
+            Assert.fail("Cell " + cell + " should be invalid");
+        }
+        catch (MarshalException e)
+        {
+            // Note that we shouldn't get anything else than a MarshalException so let other
escape and fail the test
+        }
+    }
+
+    @Test
+    public void testValidate()
+    {
+        ColumnDefinition c;
+
+        // Valid cells
+        c = fakeColumn("c", Int32Type.instance);
+        assertValid(BufferCell.live(fakeMetadata, c, 0, ByteBufferUtil.EMPTY_BYTE_BUFFER));
+        assertValid(BufferCell.live(fakeMetadata, c, 0, ByteBufferUtil.bytes(4)));
+
+        assertValid(BufferCell.expiring(c, 0, 4, 4, ByteBufferUtil.EMPTY_BYTE_BUFFER));
+        assertValid(BufferCell.expiring(c, 0, 4, 4, ByteBufferUtil.bytes(4)));
+
+        assertValid(BufferCell.tombstone(c, 0, 4));
+
+        // Invalid value (we don't all empty values for smallint)
+        c = fakeColumn("c", ShortType.instance);
+        assertInvalid(BufferCell.live(fakeMetadata, c, 0, ByteBufferUtil.EMPTY_BYTE_BUFFER));
+        // But this should be valid even though the underlying value is an empty BB (catches
bug #11618)
+        assertValid(BufferCell.tombstone(c, 0, 4));
+        // And of course, this should be valid with a proper value
+        assertValid(BufferCell.live(fakeMetadata, c, 0, ByteBufferUtil.bytes((short)4)));
+
+        // Invalid ttl
+        assertInvalid(BufferCell.expiring(c, 0, -4, 4, ByteBufferUtil.bytes(4)));
+        // Invalid local deletion times
+        assertInvalid(BufferCell.expiring(c, 0, 4, -4, ByteBufferUtil.bytes(4)));
+        assertInvalid(BufferCell.expiring(c, 0, 4, Cell.NO_DELETION_TIME, ByteBufferUtil.bytes(4)));
+
+        c = fakeColumn("c", MapType.getInstance(Int32Type.instance, Int32Type.instance, true));
+        // Valid cell path
+        assertValid(BufferCell.live(fakeMetadata, c, 0, ByteBufferUtil.bytes(4), CellPath.create(ByteBufferUtil.bytes(4))));
+        // Invalid cell path (int values should be 0 or 4 bytes)
+        assertInvalid(BufferCell.live(fakeMetadata, c, 0, ByteBufferUtil.bytes(4), CellPath.create(ByteBufferUtil.bytes((long)4))));
+    }
+
     @Test
     public void testExpiringCellReconile()
     {


Mime
View raw message