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 2013/11/22 08:46:32 UTC
git commit: Fix potential out of bounds exception during paging
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 <sy...@datastax.com>
Authored: Fri Nov 22 08:45:22 2013 +0100
Committer: Sylvain Lebresne <sy...@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)