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 2014/02/25 10:46:01 UTC
[1/2] git commit: Fix paging bug with deleted columns
Repository: cassandra
Updated Branches:
refs/heads/cassandra-2.1 6f4e0ba6b -> 3d9305320
Fix paging bug with deleted columns
patch by slebresne; reviewed by iamaleksey for CASSANDRA-6748
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo
Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/cd2c4388
Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/cd2c4388
Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/cd2c4388
Branch: refs/heads/cassandra-2.1
Commit: cd2c43884d600f9d444fbacd05f56c3581c10aa0
Parents: 3bfb764
Author: Sylvain Lebresne <sy...@datastax.com>
Authored: Tue Feb 25 10:31:14 2014 +0100
Committer: Sylvain Lebresne <sy...@datastax.com>
Committed: Tue Feb 25 10:37:18 2014 +0100
----------------------------------------------------------------------
CHANGES.txt | 1 +
.../service/pager/AbstractQueryPager.java | 8 ++--
.../service/pager/RangeSliceQueryPager.java | 13 ++++--
.../service/pager/SliceQueryPager.java | 13 +++++-
.../unit/org/apache/cassandra/SchemaLoader.java | 9 +++-
.../cassandra/service/QueryPagerTest.java | 44 ++++++++++++++++++--
6 files changed, 74 insertions(+), 14 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/cassandra/blob/cd2c4388/CHANGES.txt
----------------------------------------------------------------------
diff --git a/CHANGES.txt b/CHANGES.txt
index bfcb6a4..f3a854c 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -22,6 +22,7 @@
* Add static columns to CQL3 (CASSANDRA-6561)
* Optimize single partition batch statements (CASSANDRA-6737)
* Disallow post-query re-ordering when paging (CASSANDRA-6722)
+ * Fix potential paging bug with deleted columns (CASSANDRA-6748)
Merged from 1.2:
* Catch memtable flush exceptions during shutdown (CASSANDRA-6735)
* Fix broken streams when replacing with same IP (CASSANDRA-6622)
http://git-wip-us.apache.org/repos/asf/cassandra/blob/cd2c4388/src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java b/src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java
index 297a85f..1b4bdbd 100644
--- a/src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java
+++ b/src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java
@@ -332,13 +332,13 @@ abstract class AbstractQueryPager implements QueryPager
return Math.min(liveCount, toDiscard);
}
- protected static ByteBuffer firstName(ColumnFamily cf)
+ protected static Column firstColumn(ColumnFamily cf)
{
- return cf.iterator().next().name();
+ return cf.iterator().next();
}
- protected static ByteBuffer lastName(ColumnFamily cf)
+ protected static Column lastColumn(ColumnFamily cf)
{
- return cf.getReverseSortedColumns().iterator().next().name();
+ return cf.getReverseSortedColumns().iterator().next();
}
}
http://git-wip-us.apache.org/repos/asf/cassandra/blob/cd2c4388/src/java/org/apache/cassandra/service/pager/RangeSliceQueryPager.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/service/pager/RangeSliceQueryPager.java b/src/java/org/apache/cassandra/service/pager/RangeSliceQueryPager.java
index 1f4ba78..0df1d25 100644
--- a/src/java/org/apache/cassandra/service/pager/RangeSliceQueryPager.java
+++ b/src/java/org/apache/cassandra/service/pager/RangeSliceQueryPager.java
@@ -89,15 +89,20 @@ public class RangeSliceQueryPager extends AbstractQueryPager
protected boolean containsPreviousLast(Row first)
{
- return lastReturnedKey != null
- && lastReturnedKey.equals(first.key)
- && lastReturnedName.equals(isReversed() ? lastName(first.cf) : firstName(first.cf));
+ if (lastReturnedKey == null || !lastReturnedKey.equals(first.key))
+ return false;
+
+ // Same as SliceQueryPager, we ignore a deleted column
+ Column firstColumn = isReversed() ? lastColumn(first.cf) : firstColumn(first.cf);
+ return !first.cf.deletionInfo().isDeleted(firstColumn)
+ && firstColumn.isLive(timestamp())
+ && lastReturnedName.equals(firstColumn.name());
}
protected boolean recordLast(Row last)
{
lastReturnedKey = last.key;
- lastReturnedName = isReversed() ? firstName(last.cf) : lastName(last.cf);
+ lastReturnedName = (isReversed() ? firstColumn(last.cf) : lastColumn(last.cf)).name();
return true;
}
http://git-wip-us.apache.org/repos/asf/cassandra/blob/cd2c4388/src/java/org/apache/cassandra/service/pager/SliceQueryPager.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/service/pager/SliceQueryPager.java b/src/java/org/apache/cassandra/service/pager/SliceQueryPager.java
index cd0c069..c94f7f6 100644
--- a/src/java/org/apache/cassandra/service/pager/SliceQueryPager.java
+++ b/src/java/org/apache/cassandra/service/pager/SliceQueryPager.java
@@ -81,12 +81,21 @@ public class SliceQueryPager extends AbstractQueryPager implements SinglePartiti
protected boolean containsPreviousLast(Row first)
{
- return lastReturned != null && lastReturned.equals(isReversed() ? lastName(first.cf) : firstName(first.cf));
+ if (lastReturned == null)
+ return false;
+
+ Column firstColumn = isReversed() ? lastColumn(first.cf) : firstColumn(first.cf);
+ // Note: we only return true if the column is the lastReturned *and* it is live. If it is deleted, it is ignored by the
+ // rest of the paging code (it hasn't been counted as live in particular) and we want to act as if it wasn't there.
+ return !first.cf.deletionInfo().isDeleted(firstColumn)
+ && firstColumn.isLive(timestamp())
+ && lastReturned.equals(firstColumn.name());
}
protected boolean recordLast(Row last)
{
- lastReturned = isReversed() ? firstName(last.cf) : lastName(last.cf);
+ Column lastColumn = isReversed() ? firstColumn(last.cf) : lastColumn(last.cf);
+ lastReturned = lastColumn.name();
return true;
}
http://git-wip-us.apache.org/repos/asf/cassandra/blob/cd2c4388/test/unit/org/apache/cassandra/SchemaLoader.java
----------------------------------------------------------------------
diff --git a/test/unit/org/apache/cassandra/SchemaLoader.java b/test/unit/org/apache/cassandra/SchemaLoader.java
index 58cc52f..d554a8c 100644
--- a/test/unit/org/apache/cassandra/SchemaLoader.java
+++ b/test/unit/org/apache/cassandra/SchemaLoader.java
@@ -300,7 +300,14 @@ public class SchemaLoader
+ "k int PRIMARY KEY,"
+ "v1 text,"
+ "v2 int"
- + ")", ks_cql)));
+ + ")", ks_cql),
+
+ CFMetaData.compile("CREATE TABLE table2 ("
+ + "k text,"
+ + "c text,"
+ + "v text,"
+ + "PRIMARY KEY (k, c))", ks_cql)
+ ));
if (Boolean.parseBoolean(System.getProperty("cassandra.test.compression", "false")))
http://git-wip-us.apache.org/repos/asf/cassandra/blob/cd2c4388/test/unit/org/apache/cassandra/service/QueryPagerTest.java
----------------------------------------------------------------------
diff --git a/test/unit/org/apache/cassandra/service/QueryPagerTest.java b/test/unit/org/apache/cassandra/service/QueryPagerTest.java
index f395cf4..0645433 100644
--- a/test/unit/org/apache/cassandra/service/QueryPagerTest.java
+++ b/test/unit/org/apache/cassandra/service/QueryPagerTest.java
@@ -31,11 +31,13 @@ import org.apache.cassandra.SchemaLoader;
import org.apache.cassandra.OrderedJUnit4ClassRunner;
import org.apache.cassandra.db.*;
import org.apache.cassandra.db.filter.*;
+import org.apache.cassandra.db.marshal.CompositeType;
import org.apache.cassandra.dht.*;
import org.apache.cassandra.service.pager.*;
import org.apache.cassandra.utils.ByteBufferUtil;
import static org.junit.Assert.*;
+import static org.apache.cassandra.cql3.QueryProcessor.processInternal;
import static org.apache.cassandra.Util.range;
import static org.apache.cassandra.utils.ByteBufferUtil.bytes;
@@ -143,14 +145,25 @@ public class QueryPagerTest extends SchemaLoader
private static void assertRow(Row r, String key, String... names)
{
+ ByteBuffer[] bbs = new ByteBuffer[names.length];
+ for (int i = 0; i < names.length; i++)
+ bbs[i] = bytes(names[i]);
+ assertRow(r, key, bbs);
+ }
+
+ private static void assertRow(Row r, String key, ByteBuffer... names)
+ {
assertEquals(key, string(r.key.key));
assertNotNull(r.cf);
- assertEquals(toString(r.cf), names.length, r.cf.getColumnCount());
int i = 0;
for (Column c : r.cf)
{
- String expected = names[i++];
- assertEquals("column " + i + " doesn't match: " + toString(r.cf), expected, string(c.name()));
+ // Ignore deleted cells if we have them
+ if (!c.isLive(0))
+ continue;
+
+ ByteBuffer expected = names[i++];
+ assertEquals("column " + i + " doesn't match: " + toString(r.cf), expected, c.name());
}
}
@@ -310,4 +323,29 @@ public class QueryPagerTest extends SchemaLoader
assertTrue(pager.isExhausted());
}
+
+ @Test
+ public void SliceQueryWithTombstoneTest() throws Exception
+ {
+ // Testing for the bug of #6748
+ String keyspace = "cql_keyspace";
+ String table = "table2";
+ ColumnFamilyStore cfs = Keyspace.open(keyspace).getColumnFamilyStore(table);
+ CompositeType ct = (CompositeType)cfs.metadata.comparator;
+
+ // Insert rows but with a tombstone as last cell
+ for (int i = 0; i < 5; i++)
+ processInternal(String.format("INSERT INTO %s.%s (k, c, v) VALUES ('k%d', 'c%d', null)", keyspace, table, 0, i));
+
+ SliceQueryFilter filter = new SliceQueryFilter(ColumnSlice.ALL_COLUMNS_ARRAY, false, 100);
+ QueryPager pager = QueryPagers.localPager(new SliceFromReadCommand(keyspace, bytes("k0"), table, 0, filter));
+
+ for (int i = 0; i < 5; i++)
+ {
+ List<Row> page = pager.fetchPage(1);
+ assertEquals(toString(page), 1, page.size());
+ // The only live cell we should have each time is the row marker
+ assertRow(page.get(0), "k0", ct.decompose("c" + i, ""));
+ }
+ }
}
[2/2] git commit: Merge branch 'cassandra-2.0' into cassandra-2.1
Posted by sl...@apache.org.
Merge branch 'cassandra-2.0' into cassandra-2.1
Conflicts:
src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo
Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/3d930532
Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/3d930532
Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/3d930532
Branch: refs/heads/cassandra-2.1
Commit: 3d93053200adc722101958db19d895f6b390f160
Parents: 6f4e0ba cd2c438
Author: Sylvain Lebresne <sy...@datastax.com>
Authored: Tue Feb 25 10:45:53 2014 +0100
Committer: Sylvain Lebresne <sy...@datastax.com>
Committed: Tue Feb 25 10:45:53 2014 +0100
----------------------------------------------------------------------
CHANGES.txt | 1 +
.../service/pager/AbstractQueryPager.java | 8 ++--
.../service/pager/RangeSliceQueryPager.java | 13 ++++--
.../service/pager/SliceQueryPager.java | 13 +++++-
.../unit/org/apache/cassandra/SchemaLoader.java | 9 +++-
.../cassandra/service/QueryPagerTest.java | 44 ++++++++++++++++++--
6 files changed, 74 insertions(+), 14 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/cassandra/blob/3d930532/CHANGES.txt
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/cassandra/blob/3d930532/src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java
----------------------------------------------------------------------
diff --cc src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java
index 363aec8,1b4bdbd..9a94944
--- a/src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java
+++ b/src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java
@@@ -332,13 -332,13 +332,13 @@@ abstract class AbstractQueryPager imple
return Math.min(liveCount, toDiscard);
}
- protected static CellName firstName(ColumnFamily cf)
- protected static Column firstColumn(ColumnFamily cf)
++ protected static Cell firstCell(ColumnFamily cf)
{
- return cf.iterator().next().name();
+ return cf.iterator().next();
}
- protected static CellName lastName(ColumnFamily cf)
- protected static Column lastColumn(ColumnFamily cf)
++ protected static Cell lastCell(ColumnFamily cf)
{
- return cf.getReverseSortedColumns().iterator().next().name();
+ return cf.getReverseSortedColumns().iterator().next();
}
}
http://git-wip-us.apache.org/repos/asf/cassandra/blob/3d930532/src/java/org/apache/cassandra/service/pager/RangeSliceQueryPager.java
----------------------------------------------------------------------
diff --cc src/java/org/apache/cassandra/service/pager/RangeSliceQueryPager.java
index 9b07163,0df1d25..7e79ffe
--- a/src/java/org/apache/cassandra/service/pager/RangeSliceQueryPager.java
+++ b/src/java/org/apache/cassandra/service/pager/RangeSliceQueryPager.java
@@@ -90,15 -89,20 +90,20 @@@ public class RangeSliceQueryPager exten
protected boolean containsPreviousLast(Row first)
{
- return lastReturnedKey != null
- && lastReturnedKey.equals(first.key)
- && lastReturnedName.equals(isReversed() ? lastName(first.cf) : firstName(first.cf));
+ if (lastReturnedKey == null || !lastReturnedKey.equals(first.key))
+ return false;
+
+ // Same as SliceQueryPager, we ignore a deleted column
- Column firstColumn = isReversed() ? lastColumn(first.cf) : firstColumn(first.cf);
- return !first.cf.deletionInfo().isDeleted(firstColumn)
- && firstColumn.isLive(timestamp())
- && lastReturnedName.equals(firstColumn.name());
++ Cell firstCell = isReversed() ? lastCell(first.cf) : firstCell(first.cf);
++ return !first.cf.deletionInfo().isDeleted(firstCell)
++ && firstCell.isLive(timestamp())
++ && lastReturnedName.equals(firstCell.name());
}
protected boolean recordLast(Row last)
{
lastReturnedKey = last.key;
- lastReturnedName = isReversed() ? firstName(last.cf) : lastName(last.cf);
- lastReturnedName = (isReversed() ? firstColumn(last.cf) : lastColumn(last.cf)).name();
++ lastReturnedName = (isReversed() ? firstCell(last.cf) : lastCell(last.cf)).name();
return true;
}
http://git-wip-us.apache.org/repos/asf/cassandra/blob/3d930532/src/java/org/apache/cassandra/service/pager/SliceQueryPager.java
----------------------------------------------------------------------
diff --cc src/java/org/apache/cassandra/service/pager/SliceQueryPager.java
index fbc36e0,c94f7f6..3a8c81f
--- a/src/java/org/apache/cassandra/service/pager/SliceQueryPager.java
+++ b/src/java/org/apache/cassandra/service/pager/SliceQueryPager.java
@@@ -82,12 -81,21 +82,21 @@@ public class SliceQueryPager extends Ab
protected boolean containsPreviousLast(Row first)
{
- return lastReturned != null && lastReturned.equals(isReversed() ? lastName(first.cf) : firstName(first.cf));
+ if (lastReturned == null)
+ return false;
+
- Column firstColumn = isReversed() ? lastColumn(first.cf) : firstColumn(first.cf);
++ Cell firstCell = isReversed() ? lastCell(first.cf) : firstCell(first.cf);
+ // Note: we only return true if the column is the lastReturned *and* it is live. If it is deleted, it is ignored by the
+ // rest of the paging code (it hasn't been counted as live in particular) and we want to act as if it wasn't there.
- return !first.cf.deletionInfo().isDeleted(firstColumn)
- && firstColumn.isLive(timestamp())
- && lastReturned.equals(firstColumn.name());
++ return !first.cf.deletionInfo().isDeleted(firstCell)
++ && firstCell.isLive(timestamp())
++ && lastReturned.equals(firstCell.name());
}
protected boolean recordLast(Row last)
{
- lastReturned = isReversed() ? firstName(last.cf) : lastName(last.cf);
- Column lastColumn = isReversed() ? firstColumn(last.cf) : lastColumn(last.cf);
- lastReturned = lastColumn.name();
++ Cell lastCell = isReversed() ? firstCell(last.cf) : lastCell(last.cf);
++ lastReturned = lastCell.name();
return true;
}
http://git-wip-us.apache.org/repos/asf/cassandra/blob/3d930532/test/unit/org/apache/cassandra/SchemaLoader.java
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/cassandra/blob/3d930532/test/unit/org/apache/cassandra/service/QueryPagerTest.java
----------------------------------------------------------------------
diff --cc test/unit/org/apache/cassandra/service/QueryPagerTest.java
index 318feb9,0645433..84f51f4
--- a/test/unit/org/apache/cassandra/service/QueryPagerTest.java
+++ b/test/unit/org/apache/cassandra/service/QueryPagerTest.java
@@@ -30,8 -30,8 +30,9 @@@ import org.apache.cassandra.Util
import org.apache.cassandra.SchemaLoader;
import org.apache.cassandra.OrderedJUnit4ClassRunner;
import org.apache.cassandra.db.*;
+import org.apache.cassandra.db.composites.*;
import org.apache.cassandra.db.filter.*;
+ import org.apache.cassandra.db.marshal.CompositeType;
import org.apache.cassandra.dht.*;
import org.apache.cassandra.service.pager.*;
import org.apache.cassandra.utils.ByteBufferUtil;
@@@ -149,14 -145,25 +151,25 @@@ public class QueryPagerTest extends Sch
private static void assertRow(Row r, String key, String... names)
{
+ ByteBuffer[] bbs = new ByteBuffer[names.length];
+ for (int i = 0; i < names.length; i++)
+ bbs[i] = bytes(names[i]);
+ assertRow(r, key, bbs);
+ }
+
+ private static void assertRow(Row r, String key, ByteBuffer... names)
+ {
assertEquals(key, string(r.key.key));
assertNotNull(r.cf);
- assertEquals(toString(r.cf), names.length, r.cf.getColumnCount());
int i = 0;
- for (Column c : r.cf)
+ for (Cell c : r.cf)
{
- String expected = names[i++];
- assertEquals("column " + i + " doesn't match: " + toString(r.cf), expected, string(c.name()));
+ // Ignore deleted cells if we have them
+ if (!c.isLive(0))
+ continue;
+
+ ByteBuffer expected = names[i++];
+ assertEquals("column " + i + " doesn't match: " + toString(r.cf), expected, c.name());
}
}