cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From slebre...@apache.org
Subject [1/2] cassandra git commit: Fix assertion error when reading static on an indexed sstable
Date Wed, 23 Dec 2015 11:23:08 GMT
Repository: cassandra
Updated Branches:
  refs/heads/10903 [created] e7c6e14fd


Fix assertion error when reading static on an indexed sstable


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

Branch: refs/heads/10903
Commit: bc887acfaec939efb5b9abeb3b7e7bcf4c1c3870
Parents: 11165f4
Author: Sylvain Lebresne <sylvain@datastax.com>
Authored: Mon Dec 21 12:14:46 2015 +0100
Committer: Sylvain Lebresne <sylvain@datastax.com>
Committed: Mon Dec 21 13:41:11 2015 +0100

----------------------------------------------------------------------
 .../columniterator/AbstractSSTableIterator.java | 33 ++++----
 .../db/columniterator/SSTableIterator.java      | 38 +++-------
 .../columniterator/SSTableReversedIterator.java | 28 ++-----
 .../cql3/QueryWithIndexedSSTableTest.java       | 80 ++++++++++++++++++++
 4 files changed, 112 insertions(+), 67 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/cassandra/blob/bc887acf/src/java/org/apache/cassandra/db/columniterator/AbstractSSTableIterator.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/columniterator/AbstractSSTableIterator.java
b/src/java/org/apache/cassandra/db/columniterator/AbstractSSTableIterator.java
index 5f280d7..f103ee2 100644
--- a/src/java/org/apache/cassandra/db/columniterator/AbstractSSTableIterator.java
+++ b/src/java/org/apache/cassandra/db/columniterator/AbstractSSTableIterator.java
@@ -99,14 +99,14 @@ abstract class AbstractSSTableIterator implements SliceableUnfilteredRowIterator
 
                     // Note that this needs to be called after file != null and after the
partitionDeletion has been set, but before readStaticRow
                     // (since it uses it) so we can't move that up (but we'll be able to
simplify as soon as we drop support for the old file format).
-                    this.reader = needsReader ? createReader(indexEntry, file, true, shouldCloseFile)
: null;
+                    this.reader = needsReader ? createReader(indexEntry, file, shouldCloseFile)
: null;
                     this.staticRow = readStaticRow(sstable, file, helper, columns.fetchedColumns().statics,
isForThrift, reader == null ? null : reader.deserializer);
                 }
                 else
                 {
                     this.partitionLevelDeletion = indexEntry.deletionTime();
                     this.staticRow = Rows.EMPTY_STATIC_ROW;
-                    this.reader = needsReader ? createReader(indexEntry, file, false, shouldCloseFile)
: null;
+                    this.reader = needsReader ? createReader(indexEntry, file, shouldCloseFile)
: null;
                 }
 
                 if (reader == null && file != null && shouldCloseFile)
@@ -180,7 +180,7 @@ abstract class AbstractSSTableIterator implements SliceableUnfilteredRowIterator
         }
     }
 
-    protected abstract Reader createReader(RowIndexEntry indexEntry, FileDataInput file,
boolean isAtPartitionStart, boolean shouldCloseFile);
+    protected abstract Reader createReader(RowIndexEntry indexEntry, FileDataInput file,
boolean shouldCloseFile);
 
     public CFMetaData metadata()
     {
@@ -291,19 +291,13 @@ abstract class AbstractSSTableIterator implements SliceableUnfilteredRowIterator
         // Records the currently open range tombstone (if any)
         protected DeletionTime openMarker = null;
 
-        // !isInit means we have never seeked in the file and thus should seek before reading
anything
-        protected boolean isInit;
-
-        protected Reader(FileDataInput file, boolean isInit, boolean shouldCloseFile)
+        protected Reader(FileDataInput file, boolean shouldCloseFile)
         {
             this.file = file;
-            this.isInit = isInit;
             this.shouldCloseFile = shouldCloseFile;
 
             if (file != null)
                 createDeserializer();
-            else
-                assert !isInit;
         }
 
         private void createDeserializer()
@@ -343,12 +337,6 @@ abstract class AbstractSSTableIterator implements SliceableUnfilteredRowIterator
         {
             try
             {
-                if (!isInit)
-                {
-                    init();
-                    isInit = true;
-                }
-
                 return hasNextInternal();
             }
             catch (IOException e)
@@ -387,9 +375,6 @@ abstract class AbstractSSTableIterator implements SliceableUnfilteredRowIterator
             }
         }
 
-        // Called is hasNext() is called but we haven't been yet initialized
-        protected abstract void init() throws IOException;
-
         // Set the reader so its hasNext/next methods return values within the provided slice
         public abstract void setForSlice(Slice slice) throws IOException;
 
@@ -460,7 +445,15 @@ abstract class AbstractSSTableIterator implements SliceableUnfilteredRowIterator
         // Update the block idx based on the current reader position if we're past the current
block.
         public void updateBlock() throws IOException
         {
-            assert currentIndexIdx >= 0;
+            // If we get here with currentBlockIdx < 0, it means setToBlock() has never
been called, so it means
+            // we're about to read from the beginning of the partition, but haven't "prepared"
the IndexState yet.
+            // Do so by setting us on the first block.
+            if (currentIndexIdx < 0)
+            {
+                setToBlock(0);
+                return;
+            }
+
             while (currentIndexIdx + 1 < indexes.size() && isPastCurrentBlock())
             {
                 reader.openMarker = currentIndex().endOpenMarker;

http://git-wip-us.apache.org/repos/asf/cassandra/blob/bc887acf/src/java/org/apache/cassandra/db/columniterator/SSTableIterator.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/columniterator/SSTableIterator.java b/src/java/org/apache/cassandra/db/columniterator/SSTableIterator.java
index 3536d65..0409310 100644
--- a/src/java/org/apache/cassandra/db/columniterator/SSTableIterator.java
+++ b/src/java/org/apache/cassandra/db/columniterator/SSTableIterator.java
@@ -46,11 +46,11 @@ public class SSTableIterator extends AbstractSSTableIterator
         super(sstable, file, key, indexEntry, columns, isForThrift);
     }
 
-    protected Reader createReader(RowIndexEntry indexEntry, FileDataInput file, boolean isAtPartitionStart,
boolean shouldCloseFile)
+    protected Reader createReader(RowIndexEntry indexEntry, FileDataInput file, boolean shouldCloseFile)
     {
         return indexEntry.isIndexed()
-             ? new ForwardIndexedReader(indexEntry, file, isAtPartitionStart, shouldCloseFile)
-             : new ForwardReader(file, isAtPartitionStart, shouldCloseFile);
+             ? new ForwardIndexedReader(indexEntry, file, shouldCloseFile)
+             : new ForwardReader(file, shouldCloseFile);
     }
 
     public boolean isReverseOrder()
@@ -70,16 +70,9 @@ public class SSTableIterator extends AbstractSSTableIterator
         protected boolean sliceDone; // set to true once we know we have no more result for
the slice. This is in particular
                                      // used by the indexed reader when we know we can't
have results based on the index.
 
-        private ForwardReader(FileDataInput file, boolean isAtPartitionStart, boolean shouldCloseFile)
+        private ForwardReader(FileDataInput file, boolean shouldCloseFile)
         {
-            super(file, isAtPartitionStart, shouldCloseFile);
-        }
-
-        protected void init() throws IOException
-        {
-            // We should always have been initialized (at the beginning of the partition).
Only indexed readers may
-            // have to initialize.
-            throw new IllegalStateException();
+            super(file, shouldCloseFile);
         }
 
         public void setForSlice(Slice slice) throws IOException
@@ -95,6 +88,8 @@ public class SSTableIterator extends AbstractSSTableIterator
         // Return what should be returned at the end of this, or null if nothing should.
         private Unfiltered handlePreSliceData() throws IOException
         {
+            assert deserializer != null;
+
             // Note that the following comparison is not strict. The reason is that the only
cases
             // where it can be == is if the "next" is a RT start marker (either a '[' of
a ')[' boundary),
             // and if we had a strict inequality and an open RT marker before this, we would
issue
@@ -126,6 +121,8 @@ public class SSTableIterator extends AbstractSSTableIterator
         // if we're done with the slice.
         protected Unfiltered computeNext() throws IOException
         {
+            assert deserializer != null;
+
             if (!deserializer.hasNext() || deserializer.compareNextTo(end) > 0)
                 return null;
 
@@ -143,8 +140,6 @@ public class SSTableIterator extends AbstractSSTableIterator
             if (sliceDone)
                 return false;
 
-            assert deserializer != null;
-
             if (start != null)
             {
                 Unfiltered unfiltered = handlePreSliceData();
@@ -187,28 +182,18 @@ public class SSTableIterator extends AbstractSSTableIterator
 
         private int lastBlockIdx; // the last index block that has data for the current query
 
-        private ForwardIndexedReader(RowIndexEntry indexEntry, FileDataInput file, boolean
isAtPartitionStart, boolean shouldCloseFile)
+        private ForwardIndexedReader(RowIndexEntry indexEntry, FileDataInput file, boolean
shouldCloseFile)
         {
-            super(file, isAtPartitionStart, shouldCloseFile);
+            super(file, shouldCloseFile);
             this.indexState = new IndexState(this, sstable.metadata.comparator, indexEntry,
false);
             this.lastBlockIdx = indexState.blocksCount(); // if we never call setForSlice,
that's where we want to stop
         }
 
         @Override
-        protected void init() throws IOException
-        {
-            // If this is called, it means we're calling hasNext() before any call to setForSlice.
Which means
-            // we're reading everything from the beginning. So just set us up at the beginning
of the first block.
-            indexState.setToBlock(0);
-        }
-
-        @Override
         public void setForSlice(Slice slice) throws IOException
         {
             super.setForSlice(slice);
 
-            isInit = true;
-
             // if our previous slicing already got us the biggest row in the sstable, we're
done
             if (indexState.isDone())
             {
@@ -265,6 +250,7 @@ public class SSTableIterator extends AbstractSSTableIterator
         protected Unfiltered computeNext() throws IOException
         {
             // Our previous read might have made us cross an index block boundary. If so,
update our informations.
+            // If we read from the beginning of the partition, this is also what will initialize
the index state.
             indexState.updateBlock();
 
             // Return the next unfiltered unless we've reached the end, or we're beyond our
slice

http://git-wip-us.apache.org/repos/asf/cassandra/blob/bc887acf/src/java/org/apache/cassandra/db/columniterator/SSTableReversedIterator.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/columniterator/SSTableReversedIterator.java
b/src/java/org/apache/cassandra/db/columniterator/SSTableReversedIterator.java
index 66c32ee..14cec36 100644
--- a/src/java/org/apache/cassandra/db/columniterator/SSTableReversedIterator.java
+++ b/src/java/org/apache/cassandra/db/columniterator/SSTableReversedIterator.java
@@ -49,11 +49,11 @@ public class SSTableReversedIterator extends AbstractSSTableIterator
         super(sstable, file, key, indexEntry, columns, isForThrift);
     }
 
-    protected Reader createReader(RowIndexEntry indexEntry, FileDataInput file, boolean isAtPartitionStart,
boolean shouldCloseFile)
+    protected Reader createReader(RowIndexEntry indexEntry, FileDataInput file, boolean shouldCloseFile)
     {
         return indexEntry.isIndexed()
-             ? new ReverseIndexedReader(indexEntry, file, isAtPartitionStart, shouldCloseFile)
-             : new ReverseReader(file, isAtPartitionStart, shouldCloseFile);
+             ? new ReverseIndexedReader(indexEntry, file, shouldCloseFile)
+             : new ReverseReader(file, shouldCloseFile);
     }
 
     public boolean isReverseOrder()
@@ -66,9 +66,9 @@ public class SSTableReversedIterator extends AbstractSSTableIterator
         protected ReusablePartitionData buffer;
         protected Iterator<Unfiltered> iterator;
 
-        private ReverseReader(FileDataInput file, boolean isAtPartitionStart, boolean shouldCloseFile)
+        private ReverseReader(FileDataInput file, boolean shouldCloseFile)
         {
-            super(file, isAtPartitionStart, shouldCloseFile);
+            super(file, shouldCloseFile);
         }
 
         protected ReusablePartitionData createBuffer(int blocksCount)
@@ -100,13 +100,6 @@ public class SSTableReversedIterator extends AbstractSSTableIterator
             return new ReusablePartitionData(metadata(), partitionKey(), columns(), estimatedRowCount);
         }
 
-        protected void init() throws IOException
-        {
-            // We should always have been initialized (at the beginning of the partition).
Only indexed readers may
-            // have to initialize.
-            throw new IllegalStateException();
-        }
-
         public void setForSlice(Slice slice) throws IOException
         {
             // If we have read the data, just create the iterator for the slice. Otherwise,
read the data.
@@ -212,23 +205,16 @@ public class SSTableReversedIterator extends AbstractSSTableIterator
         // The last index block to consider for the slice
         private int lastBlockIdx;
 
-        private ReverseIndexedReader(RowIndexEntry indexEntry, FileDataInput file, boolean
isAtPartitionStart, boolean shouldCloseFile)
+        private ReverseIndexedReader(RowIndexEntry indexEntry, FileDataInput file, boolean
shouldCloseFile)
         {
-            super(file, isAtPartitionStart, shouldCloseFile);
+            super(file, shouldCloseFile);
             this.indexState = new IndexState(this, sstable.metadata.comparator, indexEntry,
true);
         }
 
-        protected void init() throws IOException
-        {
-            // This is actually a no-op, because if we call hasNext without having called
setForSlice, then ReverseReader.hasNextInternal
-            // will call setForSlice(Slice.ALL) which does the right thing.
-        }
-
         @Override
         public void setForSlice(Slice slice) throws IOException
         {
             this.slice = slice;
-            isInit = true;
 
             // if our previous slicing already got us past the beginning of the sstable,
we're done
             if (indexState.isDone())

http://git-wip-us.apache.org/repos/asf/cassandra/blob/bc887acf/test/unit/org/apache/cassandra/cql3/QueryWithIndexedSSTableTest.java
----------------------------------------------------------------------
diff --git a/test/unit/org/apache/cassandra/cql3/QueryWithIndexedSSTableTest.java b/test/unit/org/apache/cassandra/cql3/QueryWithIndexedSSTableTest.java
new file mode 100644
index 0000000..00f0ca2
--- /dev/null
+++ b/test/unit/org/apache/cassandra/cql3/QueryWithIndexedSSTableTest.java
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.cassandra.cql3;
+
+import java.util.Random;
+
+import org.junit.Test;
+
+import org.apache.cassandra.Util;
+import org.apache.cassandra.db.DecoratedKey;
+import org.apache.cassandra.db.RowIndexEntry;
+import org.apache.cassandra.io.sstable.format.SSTableReader;
+import org.apache.cassandra.utils.ByteBufferUtil;
+
+public class QueryWithIndexedSSTableTest extends CQLTester
+{
+    @Test
+    public void queryIndexedSSTableTest() throws Throwable
+    {
+        // That test reproduces the bug from CASSANDRA-10903 and the fact we have a static
column is
+        // relevant to that reproduction in particular as it forces a slightly different
code path that
+        // if there wasn't a static.
+
+        int ROWS = 1000;
+        int VALUE_LENGTH = 100;
+
+        createTable("CREATE TABLE %s (k int, t int, s text static, v text, PRIMARY KEY (k,
t))");
+
+        // We create a partition that is big enough that the underlying sstable will be indexed
+        // For that, we use a large-ish number of row, and a value that isn't too small.
+        String text = makeRandomTest(VALUE_LENGTH);
+        for (int i = 0; i < ROWS; i++)
+            execute("INSERT INTO %s(k, t, v) VALUES (?, ?, ?)", 0, i, text + i);
+
+        flush();
+        compact();
+
+        // Sanity check that we're testing what we want to test, that is that we're reading
from an indexed
+        // sstable. Note that we'll almost surely have a single indexed sstable in practice,
but it's theorically
+        // possible for a compact strategy to yield more than that and as long as one is
indexed we're pretty
+        // much testing what we want. If this check ever fails on some specific setting,
we'll have to either
+        // tweak ROWS and VALUE_LENGTH, or skip the test on those settings.
+        DecoratedKey dk = Util.dk(ByteBufferUtil.bytes(0));
+        boolean hasIndexed = false;
+        for (SSTableReader sstable : getCurrentColumnFamilyStore().getLiveSSTables())
+        {
+            RowIndexEntry indexEntry = sstable.getPosition(dk, SSTableReader.Operator.EQ);
+            hasIndexed |= indexEntry != null && indexEntry.isIndexed();
+        }
+        assert hasIndexed;
+
+        assertRowCount(execute("SELECT s FROM %s WHERE k = ?", 0), ROWS);
+    }
+
+    // Creates a random string 
+    public static String makeRandomTest(int length)
+    {
+        Random random = new Random();
+        char[] chars = new char[26];
+        int i = 0;
+        for (char c = 'a'; c <= 'z'; c++)
+            chars[i++] = c;
+        return new String(chars);
+    }
+}


Mime
View raw message