Updated Branches: refs/heads/trunk fdbddc132 -> ac926233f
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/trunk Commit: 3c9760bdb986f6c2430adfc13c86ecb75c3246ac Parents: 1ca459d Author: Sylvain Lebresne <sylv...@datastax.com> Authored: Fri Nov 22 08:45:22 2013 +0100 Committer: Sylvain Lebresne <sylv...@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)