mjsax commented on code in PR #18771:
URL: https://github.com/apache/kafka/pull/18771#discussion_r1938113387
##########
streams/src/main/java/org/apache/kafka/streams/state/internals/MeteredKeyValueStore.java:
##########
@@ -156,11 +156,8 @@ private void registerMetrics() {
(config, now) -> numOpenIterators.sum());
StateStoreMetrics.addOldestOpenIteratorGauge(taskId.toString(),
metricsScope, name(), streamsMetrics,
(config, now) -> {
- try {
- return openIterators.isEmpty() ? null :
openIterators.first().startTimestamp();
- } catch (final NoSuchElementException ignored) {
- return null;
- }
+ final Iterator<MeteredIterator> openIteratorsIterator =
openIterators.iterator();
+ return openIteratorsIterator.hasNext() ?
openIteratorsIterator.next().startTimestamp() : null;
Review Comment:
Can you explain how this avoids the race condition that you actually hit in
3.9.0 and the code in `trunk` already fixes? It actually seems to re-introduce
the race condition?
Even if `openIterators` is a `ConcurrentSkipListSet`, there is still a race
between `hasNext()` and `next()` and even after `hasNext()` returns `true`, the
last iterator could be close and remove from `openIterators` concurrently, and
`next()` would still throw an `NoSuchElementException`?
We had some discussion on the original PR about this question, too, and
concluded that we do not want to introduce heavy-weight locking, but just catch
and swallow `NoSuchElementException` in case it happens:
https://github.com/apache/kafka/pull/17713/files#r1833310356
I am not a fan of the try-catch-swallow approach, and if we did miss
anything, I would be very happy to change the code. But it seems your PR does
not do the trick.
Thoughts?
--
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
