adixitconfluent commented on code in PR #22780:
URL: https://github.com/apache/kafka/pull/22780#discussion_r3549286799
##########
core/src/main/java/kafka/server/share/DelayedShareFetch.java:
##########
@@ -290,19 +381,17 @@ private void
processAcquiredTopicPartitionsForLocalLogFetch(LinkedHashMap<TopicI
));
}
});
-
- shareFetch.maybeComplete(ShareFetchUtils.processFetchResponse(
- shareFetch,
- shareFetchPartitionDataList,
- sharePartitions,
- metadataProvider,
- exceptionHandler
- ));
+ completionConsumer.accept(shareFetchPartitionDataList);
Review Comment:
When `completeRemoteStorageShareFetchRequest` triggers
`readAndProcessFetchResultCompletion`, the flow is:
`readAndProcessFetchResultCompletion` → r`eadFuture.whenComplete` →
`processFetchResultAndComplete` releases locks for
`acquiredNonRemoteFetchTopicPartitionData` in its finally block
Then the completionConsumer callback calls `completeRemoteFetchRequest`,
which in its finally block calls
`releasePartitionLocksAndAddToActionQueue(partitionsAcquired.keySet(), ...)`.
Since partitionsAcquired contains the remote fetch partitions (not the
non-remote ones), this should be fine for lock release.
However, there's a subtle ordering issue: `processFetchResultAndComplete`
calls `completionConsumer.accept()` inside the try block, which calls
`completeRemoteFetchRequest` → `shareFetch.maybeComplete()`. If `maybeComplete`
succeeds but then completeRemoteFetchRequest's finally block throws, the
exception propagates up into processFetchResultAndComplete's catch block, which
calls `handleFetchException` on an already-completed request.
Both `maybeComplete` and `maybeCompleteWithException` have if
(isCompleted()) return; guards, so calling `handleFetchException` on an
already-completed request is a no-op for the response. However,
handleFetchException also calls `exceptionHandler.accept()` before the
`maybeCompleteWithException` check. That call goes through unconditionally and
could remove share partitions from the cache even though the request succeeded.
Hence, a successful response gets sent to the client, but the share
partition(s) may get unnecessarily evicted from the cache due to the spurious
exception handler call.
--
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]
