You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by sl...@apache.org on 2016/05/02 12:18:31 UTC
[1/3] cassandra git commit: Don't try to validate values for cell
tombstones
Repository: cassandra
Updated Branches:
refs/heads/cassandra-3.0 c08eeafdc -> de228c77b
refs/heads/trunk a62f70dd1 -> b6cc43e87
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/de228c77
Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/de228c77
Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/de228c77
Branch: refs/heads/cassandra-3.0
Commit: de228c77b0dc17cadb77a676883c4045cfc43332
Parents: c08eeaf
Author: Sylvain Lebresne <sy...@datastax.com>
Authored: Wed Apr 20 15:57:48 2016 +0200
Committer: Sylvain Lebresne <sy...@datastax.com>
Committed: Mon May 2 12:17:44 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/de228c77/CHANGES.txt
----------------------------------------------------------------------
diff --git a/CHANGES.txt b/CHANGES.txt
index 95f450b..1f715f4 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)
* Fix queries with filtering on counter columns (CASSANDRA-11629)
* Improve tombstone printing in sstabledump (CASSANDRA-11655)
* Fix paging for range queries where all clustering columns are specified (CASSANDRA-11669)
http://git-wip-us.apache.org/repos/asf/cassandra/blob/de228c77/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/de228c77/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()
{
[3/3] cassandra git commit: Merge branch 'cassandra-3.0' into trunk
Posted by sl...@apache.org.
Merge branch 'cassandra-3.0' into trunk
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo
Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/b6cc43e8
Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/b6cc43e8
Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/b6cc43e8
Branch: refs/heads/trunk
Commit: b6cc43e879d73d593957416e04dfc4af0f14cf19
Parents: a62f70d de228c7
Author: Sylvain Lebresne <sy...@datastax.com>
Authored: Mon May 2 12:18:14 2016 +0200
Committer: Sylvain Lebresne <sy...@datastax.com>
Committed: Mon May 2 12:18:14 2016 +0200
----------------------------------------------------------------------
----------------------------------------------------------------------
[2/3] cassandra git commit: Don't try to validate values for cell
tombstones
Posted by sl...@apache.org.
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/de228c77
Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/de228c77
Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/de228c77
Branch: refs/heads/trunk
Commit: de228c77b0dc17cadb77a676883c4045cfc43332
Parents: c08eeaf
Author: Sylvain Lebresne <sy...@datastax.com>
Authored: Wed Apr 20 15:57:48 2016 +0200
Committer: Sylvain Lebresne <sy...@datastax.com>
Committed: Mon May 2 12:17:44 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/de228c77/CHANGES.txt
----------------------------------------------------------------------
diff --git a/CHANGES.txt b/CHANGES.txt
index 95f450b..1f715f4 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)
* Fix queries with filtering on counter columns (CASSANDRA-11629)
* Improve tombstone printing in sstabledump (CASSANDRA-11655)
* Fix paging for range queries where all clustering columns are specified (CASSANDRA-11669)
http://git-wip-us.apache.org/repos/asf/cassandra/blob/de228c77/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/de228c77/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()
{