dimitarndimitrov commented on code in PR #17184:
URL: https://github.com/apache/kafka/pull/17184#discussion_r1763405483
##########
group-coordinator/src/main/java/org/apache/kafka/coordinator/group/metrics/HdrHistogram.java:
##########
@@ -70,19 +69,20 @@ public HdrHistogram(
) {
this.maxSnapshotAgeMs = maxSnapshotAgeMs;
recorder = new Recorder(highestTrackableValue,
numberOfSignificantValueDigits);
- this.timestampedHistogramSnapshot = new AtomicReference<>(new
Timestamped<>(0, null));
+ this.timestampedHistogramSnapshot = new Timestamped<>(0, null);
}
private Histogram latestHistogram(long now) {
- Timestamped<Histogram> latest = timestampedHistogramSnapshot.get();
- while (now - latest.timestamp > maxSnapshotAgeMs) {
- Histogram currentSnapshot = recorder.getIntervalHistogram();
- boolean updatedLatest = timestampedHistogramSnapshot.compareAndSet(
- latest, new Timestamped<>(now, currentSnapshot));
-
- latest = timestampedHistogramSnapshot.get();
- if (updatedLatest) {
- break;
Review Comment:
I made some improvements to the test I mentioned
[here](https://github.com/apache/kafka/pull/17184#issuecomment-2347304340), and
as a result it now runs much faster (well under a second on my development
machine) and fails reliably without this change (failed 500 out of 500 attempts
locally when run via Gradle).
After these improvements I prefer the aforementioned test over the one here,
as the latter requires changes in the histogram code, is still not
deterministically proving things if we are going to make it pass on a timeout,
and is a bit harder to understand.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
