cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From slebre...@apache.org
Subject git commit: Fix potential out of bounds exception during paging
Date Fri, 22 Nov 2013 07:46:32 GMT
Updated Branches:
  refs/heads/cassandra-2.0 1ca459d14 -> 3c9760bdb


Fix potential out of bounds exception during paging

patch by slebresne; reviewed by iamaleksey for CASSANDRA-6333


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

Branch: refs/heads/cassandra-2.0
Commit: 3c9760bdb986f6c2430adfc13c86ecb75c3246ac
Parents: 1ca459d
Author: Sylvain Lebresne <sylvain@datastax.com>
Authored: Fri Nov 22 08:45:22 2013 +0100
Committer: Sylvain Lebresne <sylvain@datastax.com>
Committed: Fri Nov 22 08:45:22 2013 +0100

----------------------------------------------------------------------
 CHANGES.txt                                     |  8 +-
 .../service/pager/AbstractQueryPager.java       | 80 +++++++++++++-------
 2 files changed, 56 insertions(+), 32 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/cassandra/blob/3c9760bd/CHANGES.txt
----------------------------------------------------------------------
diff --git a/CHANGES.txt b/CHANGES.txt
index 24d14ee..8163c94 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -1,8 +1,3 @@
-2.0.4
- * remove RF from nodetool ring output (CASSANDRA-6289)
- * fix attempting to flush empty rows (CASSANDRA-6374)
-
-
 2.0.3
  * Fix FD leak on slice read path (CASSANDRA-6275)
  * Cancel read meter task when closing SSTR (CASSANDRA-6358)
@@ -32,6 +27,9 @@
  * Fix paging with reversed slices (CASSANDRA-6343)
  * Set minTimestamp correctly to be able to drop expired sstables (CASSANDRA-6337)
  * Support NaN and Infinity as float literals (CASSANDRA-6003)
+ * Remove RF from nodetool ring output (CASSANDRA-6289)
+ * Fix attempting to flush empty rows (CASSANDRA-6374)
+ * Fix potential out of bounds exception when paging (CASSANDRA-6333)
 Merged from 1.2:
  * Optimize FD phi calculation (CASSANDRA-6386)
  * Improve initial FD phi estimate when starting up (CASSANDRA-6385)

http://git-wip-us.apache.org/repos/asf/cassandra/blob/3c9760bd/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 d040203..9372665 100644
--- a/src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java
+++ b/src/java/org/apache/cassandra/service/pager/AbstractQueryPager.java
@@ -77,6 +77,16 @@ abstract class AbstractQueryPager implements QueryPager
         }
 
         int liveCount = getPageLiveCount(rows);
+
+        // Because SP.getRangeSlice doesn't trim the result (see SP.trim()), liveCount may
be greater than what asked
+        // (currentPageSize). This would throw off the paging logic so we trim the excess.
It's not extremely efficient
+        // but most of the time there should be nothing or very little to trim.
+        if (liveCount > currentPageSize)
+        {
+            rows = discardLast(rows, liveCount - currentPageSize);
+            liveCount = currentPageSize;
+        }
+
         remaining -= liveCount;
 
         // If we've got less than requested, there is no more query to do (but
@@ -166,9 +176,11 @@ abstract class AbstractQueryPager implements QueryPager
     private List<Row> discardFirst(List<Row> rows)
     {
         Row first = rows.get(0);
-        ColumnFamily newCf = isReversed()
-                           ? discardLast(first.cf)
-                           : discardFirst(first.cf);
+        ColumnFamily newCf = first.cf.cloneMeShallow();
+        int discarded = isReversed()
+                      ? discardLast(first.cf, 1, newCf)
+                      : discardFirst(first.cf, 1, newCf);
+        assert discarded == 1;
 
         int count = newCf.getColumnCount();
         List<Row> newRows = new ArrayList<Row>(count == 0 ? rows.size() - 1 :
rows.size());
@@ -181,16 +193,32 @@ abstract class AbstractQueryPager implements QueryPager
 
     private List<Row> discardLast(List<Row> rows)
     {
-        Row last = rows.get(rows.size() - 1);
-        ColumnFamily newCf = isReversed()
-                           ? discardFirst(last.cf)
-                           : discardLast(last.cf);
+        return discardLast(rows, 1);
+    }
 
-        int count = newCf.getColumnCount();
-        List<Row> newRows = new ArrayList<Row>(count == 0 ? rows.size() - 1 :
rows.size());
-        newRows.addAll(rows.subList(0, rows.size() - 1));
+    private List<Row> discardLast(List<Row> rows, int toDiscard)
+    {
+        if (toDiscard == 0)
+            return rows;
+
+        int size = rows.size();
+        DecoratedKey lastKey = null;
+        ColumnFamily lastCf = null;
+        while (toDiscard > 0)
+        {
+            Row last = rows.get(--size);
+            lastKey = last.key;
+            lastCf = last.cf.cloneMeShallow();
+            toDiscard -= isReversed()
+                       ? discardFirst(last.cf, toDiscard, lastCf)
+                       : discardLast(last.cf, toDiscard, lastCf);
+        }
+
+        int count = lastCf.getColumnCount();
+        List<Row> newRows = new ArrayList<Row>(count == 0 ? size : size+1);
+        newRows.addAll(rows.subList(0, size));
         if (count != 0)
-            newRows.add(new Row(last.key, newCf));
+            newRows.add(new Row(lastKey, lastCf));
 
         return newRows;
     }
@@ -203,64 +231,62 @@ abstract class AbstractQueryPager implements QueryPager
         return count;
     }
 
-    private ColumnFamily discardFirst(ColumnFamily cf)
+    private int discardFirst(ColumnFamily cf, int toDiscard, ColumnFamily newCf)
     {
         boolean isReversed = isReversed();
         DeletionInfo.InOrderTester tester = cf.deletionInfo().inOrderTester(isReversed);
         return isReversed
-             ? discardTail(cf, cf.reverseIterator(), tester)
-             : discardHead(cf, cf.iterator(), tester);
+             ? discardTail(cf, toDiscard, newCf, cf.reverseIterator(), tester)
+             : discardHead(cf, toDiscard, newCf, cf.iterator(), tester);
     }
 
-    private ColumnFamily discardLast(ColumnFamily cf)
+    private int discardLast(ColumnFamily cf, int toDiscard, ColumnFamily newCf)
     {
         boolean isReversed = isReversed();
         DeletionInfo.InOrderTester tester = cf.deletionInfo().inOrderTester(isReversed);
         return isReversed
-             ? discardHead(cf, cf.reverseIterator(), tester)
-             : discardTail(cf, cf.iterator(), tester);
+             ? discardHead(cf, toDiscard, newCf, cf.reverseIterator(), tester)
+             : discardTail(cf, toDiscard, newCf, cf.iterator(), tester);
     }
 
-    private ColumnFamily discardHead(ColumnFamily cf, Iterator<Column> iter, DeletionInfo.InOrderTester
tester)
+    private int discardHead(ColumnFamily cf, int toDiscard, ColumnFamily copy, Iterator<Column>
iter, DeletionInfo.InOrderTester tester)
     {
-        ColumnFamily copy = cf.cloneMeShallow();
         ColumnCounter counter = columnCounter();
 
-        // Discard the first live
+        // Discard the first 'toDiscard' live
         while (iter.hasNext())
         {
             Column c = iter.next();
             counter.count(c, tester);
-            if (counter.live() > 1)
+            if (counter.live() > toDiscard)
             {
                 copy.addColumn(c);
                 while (iter.hasNext())
                     copy.addColumn(iter.next());
             }
         }
-        return copy;
+        return Math.min(counter.live(), toDiscard);
     }
 
-    private ColumnFamily discardTail(ColumnFamily cf, Iterator<Column> iter, DeletionInfo.InOrderTester
tester)
+    private int discardTail(ColumnFamily cf, int toDiscard, ColumnFamily copy, Iterator<Column>
iter, DeletionInfo.InOrderTester tester)
     {
-        ColumnFamily copy = cf.cloneMeShallow();
         // Redoing the counting like that is not extremely efficient.
         // This is called only for reversed slices or in the case of a race between
         // paging and a deletion (pretty unlikely), so this is probably acceptable.
         int liveCount = columnCounter().countAll(cf).live();
 
         ColumnCounter counter = columnCounter();
-        // Discard the first live
+        // Discard the last 'toDiscard' live (so stop adding as sound as we're past 'liveCount
- toDiscard')
         while (iter.hasNext())
         {
             Column c = iter.next();
             counter.count(c, tester);
-            if (counter.live() >= liveCount)
+            if (counter.live() > liveCount - toDiscard)
                 break;
 
             copy.addColumn(c);
         }
-        return copy;
+        return Math.min(liveCount, toDiscard);
     }
 
     protected static ByteBuffer firstName(ColumnFamily cf)


Mime
View raw message