msn-tldr commented on code in PR #15323:
URL: https://github.com/apache/kafka/pull/15323#discussion_r1481455003
##########
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java:
##########
@@ -647,27 +647,27 @@ private long batchReady(boolean exhausted, TopicPartition
part, Node leader,
}
/**
- * Iterate over partitions to see which one have batches ready and collect
leaders of those partitions
- * into the set of ready nodes. If partition has no leader, add the topic
to the set of topics with
- * no leader. This function also calculates stats for adaptive
partitioning.
+ * Iterate over partitions to see which one have batches ready and collect
leaders of those
+ * partitions into the set of ready nodes. If partition has no leader,
add the topic to the set
+ * of topics with no leader. This function also calculates stats for
adaptive partitioning.
*
- * @param metadata The cluster metadata
- * @param nowMs The current time
- * @param topic The topic
- * @param topicInfo The topic info
+ * @param cluster The cluster metadata
+ * @param nowMs The current time
+ * @param topic The topic
+ * @param topicInfo The topic info
* @param nextReadyCheckDelayMs The delay for next check
- * @param readyNodes The set of ready nodes (to be filled in)
- * @param unknownLeaderTopics The set of topics with no leader (to be
filled in)
+ * @param readyNodes The set of ready nodes (to be filled in)
+ * @param unknownLeaderTopics The set of topics with no leader (to be
filled in)
* @return The delay for next check
*/
- private long partitionReady(Metadata metadata, long nowMs, String topic,
+ private long partitionReady(Cluster cluster, long nowMs, String topic,
Review Comment:
@hachikuji
Thanks for pointing it out. As it turns out I don't need to extend the
public api of `Cluster` in order to get epoch. So internal usage doesn't change
Cluster's api anymore.
> We have been trying to reduce the reliance on Cluster internally because
it is public.
This could be achieved by created a forwarding "internal" class
`ClusterView` that uses `Cluster` by composition offering the same api. Then
`client` code can be refactored to use `ClusterInternal`. That way future
extensions of `Cluster`'s public api for internal use-cases could be prevented
by making them in `ClusterView`.
But this is going to be a size-able refactor, how about keeping it separate
from this PR? As the intention of this PR is to fix the perf bug, cherry-pick
it to other branches.
##########
clients/src/main/java/org/apache/kafka/clients/producer/internals/RecordAccumulator.java:
##########
@@ -647,27 +647,27 @@ private long batchReady(boolean exhausted, TopicPartition
part, Node leader,
}
/**
- * Iterate over partitions to see which one have batches ready and collect
leaders of those partitions
- * into the set of ready nodes. If partition has no leader, add the topic
to the set of topics with
- * no leader. This function also calculates stats for adaptive
partitioning.
+ * Iterate over partitions to see which one have batches ready and collect
leaders of those
+ * partitions into the set of ready nodes. If partition has no leader,
add the topic to the set
+ * of topics with no leader. This function also calculates stats for
adaptive partitioning.
*
- * @param metadata The cluster metadata
- * @param nowMs The current time
- * @param topic The topic
- * @param topicInfo The topic info
+ * @param cluster The cluster metadata
+ * @param nowMs The current time
+ * @param topic The topic
+ * @param topicInfo The topic info
* @param nextReadyCheckDelayMs The delay for next check
- * @param readyNodes The set of ready nodes (to be filled in)
- * @param unknownLeaderTopics The set of topics with no leader (to be
filled in)
+ * @param readyNodes The set of ready nodes (to be filled in)
+ * @param unknownLeaderTopics The set of topics with no leader (to be
filled in)
* @return The delay for next check
*/
- private long partitionReady(Metadata metadata, long nowMs, String topic,
+ private long partitionReady(Cluster cluster, long nowMs, String topic,
Review Comment:
@hachikuji
Thanks for pointing it out. As it turns out I don't need to extend the
public api of `Cluster` in order to get epoch. So internal usage doesn't change
Cluster's api anymore.
> We have been trying to reduce the reliance on Cluster internally because
it is public.
This could be achieved by created a forwarding "internal" class
`ClusterView` that uses `Cluster` by composition offering the same api. Then
`client` code can be refactored to use `ClusterView`. That way future
extensions of `Cluster`'s public api for internal use-cases could be prevented
by making them in `ClusterView`.
But this is going to be a size-able refactor, how about keeping it separate
from this PR? As the intention of this PR is to fix the perf bug, cherry-pick
it to other branches.
--
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
