stevenschlansker commented on code in PR #18771:
URL: https://github.com/apache/kafka/pull/18771#discussion_r1938120110
##########
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:
Thanks for considering this PR @mjsax . I admit I do not have too much
experience with `ConcurrentSkipListSet` (and the `ConcurrentSkipListMap`
underlying it), but they promise:
> Iterators and spliterators are weakly consistent.
where weakly consistent is defined:
> Most concurrent Collection implementations (including most Queues) also
differ from the usual java.util conventions in that their Iterators and
Spliterators provide weakly consistent rather than fast-fail traversal:
> they may proceed concurrently with other operations
> they will never throw ConcurrentModificationException
> they are guaranteed to traverse elements as they existed upon
construction exactly once, and may (but are not guaranteed to) reflect any
modifications subsequent to construction.
I take this to mean that by taking a single `openIterators.iterator()` we
then get a weakly consistent handle on the structure, and will either observe
an element or not, but such an iterator should never run into the case where
`hasNext()` returns `true` but then `next()` throws (which would not be
"consistent" to me).
I admit the docs stating "they may proceed concurrently with other
operations" "won't throw CME" isn't 100% clear on this point about the slightly
different NoSuchElementException.
Assuming my analysis is correct, this is an improvement because the old code
`isEmpty() + first()` is taking two "weak snapshots" of the data structure that
are not consistent, but `iterator() { hasNext() + next() }` takes a single weak
snapshot.
If this isn't sufficiently convincing, I'm fine to drop this PR and move on
with life :)
--
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]
