cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From slebre...@apache.org
Subject [1/2] git commit: Fix paging bug with deleted columns
Date Tue, 25 Feb 2014 09:46:01 GMT
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 <sylvain@datastax.com>
Authored: Tue Feb 25 10:31:14 2014 +0100
Committer: Sylvain Lebresne <sylvain@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, ""));
+        }
+    }
 }


Mime
View raw message