Re: [PR] CASSANDRA-19365: fix races in DecayingEstimatedHistogramReservoir [cassandra]

["maedhroz (via GitHub)" <gi...@apache.org>]

maedhroz commented on code in PR #3102:
URL: https://github.com/apache/cassandra/pull/3102#discussion_r1579936646


##########
src/java/org/apache/cassandra/metrics/DecayingEstimatedHistogramReservoir.java:
##########
@@ -430,19 +430,64 @@ public void rebase(EstimatedHistogramReservoirSnapshot snapshot)
             }
         }
 
-        this.decayLandmark = snapshot.snapshotLandmark;
+        DecayingBuckets newDecayingBuckets = new DecayingBuckets(snapshot.snapshotLandmark);
         for (int i = 0; i < size(); i++)
         {
             // set rebased values in the first stripe and clear out all other data
-            decayingBuckets.set(stripedIndex(i, 0), snapshot.decayingBuckets[i]);
+            newDecayingBuckets.buckets.set(stripedIndex(i, 0), snapshot.decayingBuckets[i]);
             buckets.set(stripedIndex(i, 0), snapshot.values[i]);
             for (int stripe = 1; stripe < nStripes; stripe++)
             {
-                decayingBuckets.set(stripedIndex(i, stripe), 0);
+                newDecayingBuckets.buckets.set(stripedIndex(i, stripe), 0);
                 buckets.set(stripedIndex(i, stripe), 0);
             }
         }
+        decayingBuckets = newDecayingBuckets;
+    }
+
+    /**
+     * The DecayingBuckets class provides a facility to prevent destruction of the reservoir internal
+     * state (CASSANDRA-19365) from occurring.
+     * The root cause of CASSANDRA-19365 is lack of synchronization between udpates and rescaling.
+     * This class lets us retain lack of synchronization (for performance reasons) while changing
+     * the race condition effect from destructive to benign.
+     * <p/>
+     * DecayingBuckets class encapsulates the decaying buckets and the decay landmark together.
+     * The decay landmark is immutable and so the values in the buckets are consistent with the landmark
+     * at any given time. In particular, every update is always given a weight that's consistent
+     * with weights given to other updates of the same DecayingBuckets.
+     * <p/>
+     * Additionally, the class allows creating snapshots of the reservoir without risking that
+     * the snapshot uses data from a half-rescaled reservoir.
+     * <p/>
+     * DecayingBuckets are not suitable for thread-safe rescaling in place. Instead,
+     * a new instance of DecayingBuckets should be created when rescaling is needed.
+     * <p/>
+     * This class does NOT provide thread safety guarantees for updates vs rescale
+     * As a matter of fact, there is a known race condition between update and rescale
+     * (see {@link #rescale(long)}).
+     * <p/>
+     */
+    private class DecayingBuckets

Review Comment:
   nit: Could always throw a `@NotThreadSafe` annotation on this.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org