cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From slebre...@apache.org
Subject [2/6] cassandra git commit: Lost counter writes in compact table and static columns
Date Thu, 28 Jul 2016 08:22:27 GMT
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.9
Commit: a11f210f133ef8026e278381d3a0b703ff7165fb
Parents: d6232e0
Author: Sylvain Lebresne <sylvain@datastax.com>
Authored: Wed Jul 27 17:15:18 2016 +0200
Committer: Sylvain Lebresne <sylvain@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()


Mime
View raw message