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 2015/09/04 11:16:32 UTC
[2/3] cassandra git commit: Fix backward compatibility issue due to
AbstractBounds serialization bug
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/trunk
Commit: 24682d21d22991deb300ec48527881a532c25c42
Parents: f009272
Author: Sylvain Lebresne <sy...@datastax.com>
Authored: Tue Sep 1 17:56:54 2015 +0200
Committer: Sylvain Lebresne <sy...@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));
+ }
+}