AndrewJSchofield commented on code in PR #17055:
URL: https://github.com/apache/kafka/pull/17055#discussion_r1739412240
##########
core/src/main/java/kafka/server/share/SharePartitionManager.java:
##########
@@ -683,19 +683,22 @@ CompletableFuture<Map<TopicIdPartition, PartitionData>>
processFetchResponse(
}
// Visible for testing.
- void releaseFetchQueueAndPartitionsLock(String groupId,
Set<TopicIdPartition> topicIdPartitions) {
- topicIdPartitions.forEach(tp ->
partitionCacheMap.get(sharePartitionKey(groupId, tp)).releaseFetchLock());
- releaseProcessFetchQueueLock();
+ void releaseProcessFetchQueueLock() {
+ processFetchQueueLock.set(false);
}
- private void releaseProcessFetchQueueLock() {
- processFetchQueueLock.set(false);
+ private void releasePartitionsLock(String groupId, Set<TopicIdPartition>
topicIdPartitions) {
Review Comment:
nit: `releasePartitionLocks` would be a better name.
##########
core/src/main/java/kafka/server/share/SharePartitionManager.java:
##########
@@ -521,107 +521,107 @@ void maybeProcessFetchQueue() {
// The queue is already being processed hence avoid re-triggering.
return;
}
+ processFetchQueue();
+ }
Review Comment:
I believe that `processFetchQueue()` always releases the
`processFetchQueueLock` is the final step when it returns. I would prefer to
see the lock released in a try-finally in this `maybeProcessFetchQueue` method
to make the code more resilient to change. Having the lock and unlock in
separate methods seems unwise, and I believe the code structure makes this
quite simple.
--
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
