apoorvmittal10 commented on code in PR #22780:
URL: https://github.com/apache/kafka/pull/22780#discussion_r3550187810
##########
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:
> readAndProcessFetchResultCompletion → readFuture.whenComplete →
processFetchResultAndComplete releases locks for
acquiredNonRemoteFetchTopicPartitionData in its finally block
Prior releasing the locks for `acquiredNonRemoteFetchTopicPartitionData` the
`completionConsumer.accept` will be triggered as I think that's how try/finally
works. Let me know if I am missing something. Hence, the lock for
`acquiredNonRemoteFetchTopicPartitionData` is still held while
`completionConsumer.accept` is triggered.
> completeRemoteFetchRequest's finally block throws, the exception
propagates up into processFetchResultAndComplete's catch block, which calls
handleFetchException on an already-completed request.
Well there isn't anything to be thrown in finally block there, the locks are
released. If there happens to be a runtime exception then it's a bug which
shall be fixed. The locks release is not expected to throw exception otherwise
we have bigger things to worry like pending locks.
> which calls handleFetchException on an already-completed request
It should ideally not happen else we have a bug for an exception which is
not expected from releasing locks, but if it happens as well then that's a
no-op for future completion, as you also mentioned.
> 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.
But if there are exceptions then the exception handler will do the needful,
all depends on the exception type. It will not be an unnecessarily eviction if
it needs to happen.
--
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]
