Mmuzaf commented on code in PR #3102:
URL: https://github.com/apache/cassandra/pull/3102#discussion_r1713951528
##########
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
+ {
+ private final long decayLandmark;
+ private final AtomicLongArray buckets;
+ public DecayingBuckets(long decayLandmark)
Review Comment:
I would make the `DecayingBuckets` class static and pass
`(bucketOffsets.length + 1) * nStripes` as a parameter to it to outline that
`buckets` and `DecayingBuckets` have the same length regardless of where they
are used.
In other cases, I must fall into the `DecayingBuckets` class to understand
what it does.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
