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 <[email protected]> Authored: Tue Sep 1 17:56:54 2015 +0200 Committer: Sylvain Lebresne <[email protected]> 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)); + } +}
