Repository: cassandra Updated Branches: refs/heads/cassandra-3.0 d6232e04d -> a11f210f1 refs/heads/cassandra-3.9 d22e3908b -> f0fd9ad6c refs/heads/trunk 5051c0f6e -> 9b44a30bb
Lost counter writes in compact table and static columns patch by Sylvain Lebresne; reviewed by Aleksey Yeschenko for CASSANDRA-12219 Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/a11f210f Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/a11f210f Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/a11f210f Branch: refs/heads/cassandra-3.0 Commit: a11f210f133ef8026e278381d3a0b703ff7165fb Parents: d6232e0 Author: Sylvain Lebresne <sylv...@datastax.com> Authored: Wed Jul 27 17:15:18 2016 +0200 Committer: Sylvain Lebresne <sylv...@datastax.com> Committed: Thu Jul 28 10:19:09 2016 +0200 ---------------------------------------------------------------------- CHANGES.txt | 1 + .../apache/cassandra/cql3/UpdateParameters.java | 17 +++++++++-------- .../apache/cassandra/db/CounterMutation.java | 3 ++- .../db/partitions/PartitionUpdate.java | 20 ++++++++++++-------- 4 files changed, 24 insertions(+), 17 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/a11f210f/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index 90f1ee9..b966078 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 3.0.9 + * Lost counter writes in compact table and static columns (CASSANDRA-12219) * AssertionError with MVs on updating a row that isn't indexed due to a null value (CASSANDRA-12247) * Disable RR and speculative retry with EACH_QUORUM reads (CASSANDRA-11980) * Add option to override compaction space check (CASSANDRA-12180) http://git-wip-us.apache.org/repos/asf/cassandra/blob/a11f210f/src/java/org/apache/cassandra/cql3/UpdateParameters.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/UpdateParameters.java b/src/java/org/apache/cassandra/cql3/UpdateParameters.java index 572365b..0c58097 100644 --- a/src/java/org/apache/cassandra/cql3/UpdateParameters.java +++ b/src/java/org/apache/cassandra/cql3/UpdateParameters.java @@ -158,14 +158,15 @@ public class UpdateParameters { assert ttl == LivenessInfo.NO_TTL; - // In practice, the actual CounterId (and clock really) that we use doesn't matter, because we will - // ignore it in CounterMutation when we do the read-before-write to create the actual value that is - // applied. In other words, this is not the actual value that will be written to the memtable - // because this will be replaced in CounterMutation.updateWithCurrentValue(). - // As an aside, since we don't care about the CounterId/clock, we used to only send the incremement, - // but that makes things a bit more complex as this means we need to be able to distinguish inside - // PartitionUpdate between counter updates that has been processed by CounterMutation and those that - // haven't. + // Because column is a counter, we need the value to be a CounterContext. However, we're only creating a + // "counter update", which is a temporary state until we run into 'CounterMutation.updateWithCurrentValue()' + // which does the read-before-write and sets the proper CounterId, clock and updated value. + // + // We thus create a "fake" local shard here. The CounterId/clock used don't matter as this is just a temporary + // state that will be replaced when processing the mutation in CounterMutation, but the reason we use a 'local' + // shard is due to the merging rules: if a user includes multiple updates to the same counter in a batch, those + // multiple updates will be merged in the PartitionUpdate *before* they even reach CounterMutation. So we need + // such update to be added together, and that's what a local shard gives us. builder.addCell(BufferCell.live(metadata, column, timestamp, CounterContext.instance().createLocal(increment))); } http://git-wip-us.apache.org/repos/asf/cassandra/blob/a11f210f/src/java/org/apache/cassandra/db/CounterMutation.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/CounterMutation.java b/src/java/org/apache/cassandra/db/CounterMutation.java index 6818513..8aafa5c 100644 --- a/src/java/org/apache/cassandra/db/CounterMutation.java +++ b/src/java/org/apache/cassandra/db/CounterMutation.java @@ -238,7 +238,8 @@ public class CounterMutation implements IMutation BTreeSet.Builder<Clustering> names = BTreeSet.builder(cfs.metadata.comparator); for (PartitionUpdate.CounterMark mark : marks) { - names.add(mark.clustering()); + if (mark.clustering() != Clustering.STATIC_CLUSTERING) + names.add(mark.clustering()); if (mark.path() == null) builder.add(mark.column()); else http://git-wip-us.apache.org/repos/asf/cassandra/blob/a11f210f/src/java/org/apache/cassandra/db/partitions/PartitionUpdate.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/partitions/PartitionUpdate.java b/src/java/org/apache/cassandra/db/partitions/PartitionUpdate.java index 6331440..2a881a3 100644 --- a/src/java/org/apache/cassandra/db/partitions/PartitionUpdate.java +++ b/src/java/org/apache/cassandra/db/partitions/PartitionUpdate.java @@ -484,19 +484,23 @@ public class PartitionUpdate extends AbstractBTreePartition assert metadata().isCounter(); maybeBuild(); // We will take aliases on the rows of this update, and update them in-place. So we should be sure the - // update is no immutable for all intent and purposes. + // update is now immutable for all intent and purposes. canReOpen = false; - List<CounterMark> l = new ArrayList<>(); + List<CounterMark> marks = new ArrayList<>(); + addMarksForRow(staticRow(), marks); for (Row row : this) + addMarksForRow(row, marks); + return marks; + } + + private void addMarksForRow(Row row, List<CounterMark> marks) + { + for (Cell cell : row.cells()) { - for (Cell cell : row.cells()) - { - if (cell.isCounterCell()) - l.add(new CounterMark(row, cell.column(), cell.path())); - } + if (cell.isCounterCell()) + marks.add(new CounterMark(row, cell.column(), cell.path())); } - return l; } private void assertNotBuilt()