adixitconfluent commented on code in PR #18959:
URL: https://github.com/apache/kafka/pull/18959#discussion_r1965474170
##########
core/src/main/java/kafka/server/share/DelayedShareFetch.java:
##########
@@ -62,9 +63,13 @@ public class DelayedShareFetch extends DelayedOperation {
private final ReplicaManager replicaManager;
private final BiConsumer<SharePartitionKey, Throwable> exceptionHandler;
private final PartitionMaxBytesStrategy partitionMaxBytesStrategy;
+ private final ShareGroupMetrics shareGroupMetrics;
+ private final Time time;
// The topic partitions that need to be completed for the share fetch
request are given by sharePartitions.
// sharePartitions is a subset of shareFetchData. The order of
insertion/deletion of entries in sharePartitions is important.
private final LinkedHashMap<TopicIdPartition, SharePartition>
sharePartitions;
+ // Tracks the start time to acquire any share partition for a fetch
request.
+ private long acquireStartTimeMs;
Review Comment:
@apoorvmittal10, in order to achieve the motive of the metric
`TopicPartitionsAcquireTimeMs`, shouldn't `acquireStartTimeMs` be assigned
`time.hiResClockMs()` whenever `tryComplete` runs and when code hits the
condition `if (partitionsAcquired.isEmpty())` in `onComplete`. Otherwise we
will be using a stale value of acquireStartTimeMs to get the current time
interval. `tryComplete` won't get triggered repeatedly unless some other
`DelayedShareFetch` is complete and interesting case of `onComplete` won't get
triggered unless timeout. So, we would be adding some extra time. Is my
understanding wrong?
--
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
