cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From slebre...@apache.org
Subject cassandra git commit: Fix backward compatibility issue due to AbstractBounds serialization bug
Date Fri, 04 Sep 2015 09:16:15 GMT
Repository: cassandra
Updated Branches:
  refs/heads/cassandra-3.0 f009272ba -> 24682d21d


Fix backward compatibility issue due to AbstractBounds serialization bug

patch by slebresne; reviewed by bdeggleston for CASSANDRA-9857


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

Branch: refs/heads/cassandra-3.0
Commit: 24682d21d22991deb300ec48527881a532c25c42
Parents: f009272
Author: Sylvain Lebresne <sylvain@datastax.com>
Authored: Tue Sep 1 17:56:54 2015 +0200
Committer: Sylvain Lebresne <sylvain@datastax.com>
Committed: Fri Sep 4 11:14:42 2015 +0200

----------------------------------------------------------------------
 CHANGES.txt                                     |  1 +
 .../org/apache/cassandra/db/ReadResponse.java   | 38 +++++++-
 .../apache/cassandra/db/ReadResponseTest.java   | 99 ++++++++++++++++++++
 3 files changed, 133 insertions(+), 5 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/cassandra/blob/24682d21/CHANGES.txt
----------------------------------------------------------------------
diff --git a/CHANGES.txt b/CHANGES.txt
index 726eb04..4d8a932 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -1,4 +1,5 @@
 3.0.0-beta2
+ * Fix backward compatibility issue due to AbstractBounds serialization bug (CASSANDRA-9857)
  * Fix startup error when upgrading nodes (CASSANDRA-10136)
  * Base table PRIMARY KEY can be assumed to be NOT NULL in MV creation (CASSANDRA-10147)
  * Improve batchlog write patch (CASSANDRA-9673)

http://git-wip-us.apache.org/repos/asf/cassandra/blob/24682d21/src/java/org/apache/cassandra/db/ReadResponse.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/db/ReadResponse.java b/src/java/org/apache/cassandra/db/ReadResponse.java
index 547e7f4..b8ffe25 100644
--- a/src/java/org/apache/cassandra/db/ReadResponse.java
+++ b/src/java/org/apache/cassandra/db/ReadResponse.java
@@ -31,6 +31,7 @@ import org.apache.cassandra.db.filter.ClusteringIndexFilter;
 import org.apache.cassandra.db.filter.ColumnFilter;
 import org.apache.cassandra.db.rows.*;
 import org.apache.cassandra.db.partitions.*;
+import org.apache.cassandra.dht.*;
 import org.apache.cassandra.io.IVersionedSerializer;
 import org.apache.cassandra.io.util.DataInputBuffer;
 import org.apache.cassandra.io.util.DataInputPlus;
@@ -221,11 +222,13 @@ public abstract class ReadResponse
      * sorted order, even if the query asks for reversed results.  Additionally,  pre-3.0
nodes do not have a notion of
      * exclusive slices on non-composite tables, so extra rows may need to be trimmed.
      */
-    private static class LegacyRemoteDataResponse extends ReadResponse
+    @VisibleForTesting
+    static class LegacyRemoteDataResponse extends ReadResponse
     {
         private final List<ImmutableBTreePartition> partitions;
 
-        private LegacyRemoteDataResponse(List<ImmutableBTreePartition> partitions)
+        @VisibleForTesting
+        LegacyRemoteDataResponse(List<ImmutableBTreePartition> partitions)
         {
             super(null); // we never serialize LegacyRemoteDataResponses, so we don't care
about the metadata
             this.partitions = partitions;
@@ -233,6 +236,31 @@ public abstract class ReadResponse
 
         public UnfilteredPartitionIterator makeIterator(CFMetaData metadata, final ReadCommand
command)
         {
+            // Due to a bug in the serialization of AbstractBounds, anything that isn't a
Range is understood by pre-3.0 nodes
+            // as a Bound, which means IncludingExcludingBounds and ExcludingBounds responses
may include keys they shouldn't.
+            // So filter partitions that shouldn't be included here.
+            boolean skipFirst = false;
+            boolean skipLast = false;
+            if (!partitions.isEmpty() && command instanceof PartitionRangeReadCommand)
+            {
+                AbstractBounds<PartitionPosition> keyRange = ((PartitionRangeReadCommand)command).dataRange().keyRange();
+                boolean isExcludingBounds = keyRange instanceof ExcludingBounds;
+                skipFirst = isExcludingBounds && !keyRange.contains(partitions.get(0).partitionKey());
+                skipLast = (isExcludingBounds || keyRange instanceof IncludingExcludingBounds)
&& !keyRange.contains(partitions.get(partitions.size() - 1).partitionKey());
+            }
+
+            final List<ImmutableBTreePartition> toReturn;
+            if (skipFirst || skipLast)
+            {
+                toReturn = partitions.size() == 1
+                         ? Collections.emptyList()
+                         : partitions.subList(skipFirst ? 1 : 0, skipLast ? partitions.size()
- 1 : partitions.size());
+            }
+            else
+            {
+                toReturn = partitions;
+            }
+
             return new AbstractUnfilteredPartitionIterator()
             {
                 private int idx;
@@ -249,12 +277,13 @@ public abstract class ReadResponse
 
                 public boolean hasNext()
                 {
-                    return idx < partitions.size();
+                    return idx < toReturn.size();
                 }
 
                 public UnfilteredRowIterator next()
                 {
-                    ImmutableBTreePartition partition = partitions.get(idx++);
+                    ImmutableBTreePartition partition = toReturn.get(idx++);
+
 
                     ClusteringIndexFilter filter = command.clusteringIndexFilter(partition.partitionKey());
 
@@ -468,7 +497,6 @@ public abstract class ReadResponse
 
         public ReadResponse deserialize(DataInputPlus in, int version) throws IOException
         {
-            // Contrarily to serialize, we have to read the number of serialized partitions
here.
             int partitionCount = in.readInt();
             ArrayList<ImmutableBTreePartition> partitions = new ArrayList<>(partitionCount);
             for (int i = 0; i < partitionCount; i++)

http://git-wip-us.apache.org/repos/asf/cassandra/blob/24682d21/test/unit/org/apache/cassandra/db/ReadResponseTest.java
----------------------------------------------------------------------
diff --git a/test/unit/org/apache/cassandra/db/ReadResponseTest.java b/test/unit/org/apache/cassandra/db/ReadResponseTest.java
new file mode 100644
index 0000000..af0ec60
--- /dev/null
+++ b/test/unit/org/apache/cassandra/db/ReadResponseTest.java
@@ -0,0 +1,99 @@
+/*
+ * 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.db;
+
+import java.util.*;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import org.apache.cassandra.Util;
+import org.apache.cassandra.config.CFMetaData;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.cql3.CQLTester;
+import org.apache.cassandra.db.rows.Rows;
+import org.apache.cassandra.db.rows.UnfilteredRowIterators;
+import org.apache.cassandra.db.partitions.ImmutableBTreePartition;
+import org.apache.cassandra.db.partitions.UnfilteredPartitionIterator;
+import org.apache.cassandra.db.marshal.AsciiType;
+import org.apache.cassandra.dht.ByteOrderedPartitioner;
+import org.apache.cassandra.dht.IPartitioner;
+
+import static org.junit.Assert.assertEquals;
+
+public class ReadResponseTest extends CQLTester
+{
+    private IPartitioner partitionerToRestore;
+
+    @Before
+    public void setupPartitioner()
+    {
+        // Using an ordered partitioner to be able to predict keys order in the following
tests.
+        partitionerToRestore = DatabaseDescriptor.setPartitionerUnsafe(ByteOrderedPartitioner.instance);
+    }
+
+    @After
+    public void resetPartitioner()
+    {
+        DatabaseDescriptor.setPartitionerUnsafe(partitionerToRestore);
+    }
+
+    @Test
+    public void testLegacyResponseSkipWrongBounds()
+    {
+        createTable("CREATE TABLE %s (k text PRIMARY KEY)");
+
+        ColumnFamilyStore cfs = getCurrentColumnFamilyStore();
+
+        // Test that if a legacy response contains keys at the boundary of the requested
key range that shouldn't be present, those
+        // are properly skipped. See CASSANDRA-9857 for context.
+
+        List<ImmutableBTreePartition> responses = Arrays.asList(makePartition(cfs.metadata,
"k1"),
+                                                                makePartition(cfs.metadata,
"k2"),
+                                                                makePartition(cfs.metadata,
"k3"));
+        ReadResponse.LegacyRemoteDataResponse response = new ReadResponse.LegacyRemoteDataResponse(responses);
+
+        assertPartitions(response.makeIterator(cfs.metadata, Util.cmd(cfs).fromKeyExcl("k1").toKeyExcl("k3").build()),
"k2");
+        assertPartitions(response.makeIterator(cfs.metadata, Util.cmd(cfs).fromKeyExcl("k0").toKeyExcl("k3").build()),
"k1", "k2");
+        assertPartitions(response.makeIterator(cfs.metadata, Util.cmd(cfs).fromKeyExcl("k1").toKeyExcl("k4").build()),
"k2", "k3");
+
+        assertPartitions(response.makeIterator(cfs.metadata, Util.cmd(cfs).fromKeyIncl("k1").toKeyExcl("k3").build()),
"k1", "k2");
+        assertPartitions(response.makeIterator(cfs.metadata, Util.cmd(cfs).fromKeyIncl("k1").toKeyExcl("k4").build()),
"k1", "k2", "k3");
+    }
+
+    private void assertPartitions(UnfilteredPartitionIterator actual, String... expectedKeys)
+    {
+        int i = 0;
+        while (i < expectedKeys.length && actual.hasNext())
+        {
+            String actualKey = AsciiType.instance.getString(actual.next().partitionKey().getKey());
+            assertEquals(expectedKeys[i++], actualKey);
+        }
+
+        if (i < expectedKeys.length)
+            throw new AssertionError("Got less results than expected: " + expectedKeys[i]
+ " is not in the result");
+        if (actual.hasNext())
+            throw new AssertionError("Got more results than expected: first unexpected key
is " + AsciiType.instance.getString(actual.next().partitionKey().getKey()));
+    }
+
+    private static ImmutableBTreePartition makePartition(CFMetaData metadata, String key)
+    {
+        return ImmutableBTreePartition.create(UnfilteredRowIterators.noRowsIterator(metadata,
Util.dk(key), Rows.EMPTY_STATIC_ROW, new DeletionTime(0, 0), false));
+    }
+}


Mime
View raw message