AndrewJSchofield commented on code in PR #16083:
URL: https://github.com/apache/kafka/pull/16083#discussion_r1616865033
##########
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerRebalanceListener.java:
##########
@@ -28,37 +28,37 @@
* This is applicable when the consumer is having Kafka auto-manage group
membership. If the consumer directly assigns partitions,
* those partitions will never be reassigned and this callback is not
applicable.
* <p>
- * When Kafka is managing the group membership, a partition re-assignment will
be triggered any time the members of the group change or the subscription
+ * When Kafka is managing the group membership, a partition re-assignment will
be triggered whenever the members of the group change or the subscription
* of the members changes. This can occur when processes die, new process
instances are added or old instances come back to life after failure.
- * Partition re-assignments can also be triggered by changes affecting the
subscribed topics (e.g. when the number of partitions is
+ * Partition re-assignments can also be triggered by changing affecting the
subscribed topics (e.g. when the number of partitions is
* administratively adjusted).
* <p>
* There are many uses for this functionality. One common use is saving
offsets in a custom store. By saving offsets in
* the {@link #onPartitionsRevoked(Collection)} call we can ensure that any
time partition assignment changes
Review Comment:
It would read more clearly with a comma between "call" and "we can ensure".
##########
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerRebalanceListener.java:
##########
@@ -28,37 +28,37 @@
* This is applicable when the consumer is having Kafka auto-manage group
membership. If the consumer directly assigns partitions,
* those partitions will never be reassigned and this callback is not
applicable.
* <p>
- * When Kafka is managing the group membership, a partition re-assignment will
be triggered any time the members of the group change or the subscription
+ * When Kafka is managing the group membership, a partition re-assignment will
be triggered whenever the members of the group change or the subscription
* of the members changes. This can occur when processes die, new process
instances are added or old instances come back to life after failure.
- * Partition re-assignments can also be triggered by changes affecting the
subscribed topics (e.g. when the number of partitions is
+ * Partition re-assignments can also be triggered by changing affecting the
subscribed topics (e.g. when the number of partitions is
* administratively adjusted).
* <p>
* There are many uses for this functionality. One common use is saving
offsets in a custom store. By saving offsets in
* the {@link #onPartitionsRevoked(Collection)} call we can ensure that any
time partition assignment changes
* the offset gets saved.
* <p>
* Another use is flushing out any kind of cache of intermediate results the
consumer may be keeping. For example,
- * consider a case where the consumer is subscribed to a topic containing user
page views, and the goal is to count the
- * number of page views per user for each five minute window. Let's say the
topic is partitioned by the user id so that
+ * consider a case where the consumer is subscribing to a topic containing
user page views, and the goal is to count the
Review Comment:
I think "subscribed" is better. With "subscribing", it sounds like we are
only concerned with the time while the consumer is actually making its
subscription. An alternative might be "where the consumer subscribes to a
topic".
##########
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerRebalanceListener.java:
##########
@@ -119,15 +119,15 @@
public interface ConsumerRebalanceListener {
/**
- * A callback method the user can implement to provide handling of offset
commits to a customized store.
+ * A callback method the user can implement to provide handling of offset
commits **sent to** a customized store
Review Comment:
Also, I wonder whether this first sentence is really giving a common example
of what the callback might do. Really, the callback is to provide handling of
cleaning up resources which are being assigned to other consumers. The
customized store is a possible use.
##########
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerRebalanceListener.java:
##########
@@ -119,15 +119,15 @@
public interface ConsumerRebalanceListener {
/**
- * A callback method the user can implement to provide handling of offset
commits to a customized store.
+ * A callback method the user can implement to provide handling of offset
commits **sent to** a customized store
Review Comment:
Add "." to the end of this sentence please.
##########
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerRebalanceListener.java:
##########
@@ -28,37 +28,37 @@
* This is applicable when the consumer is having Kafka auto-manage group
membership. If the consumer directly assigns partitions,
* those partitions will never be reassigned and this callback is not
applicable.
* <p>
- * When Kafka is managing the group membership, a partition re-assignment will
be triggered any time the members of the group change or the subscription
+ * When Kafka is managing the group membership, a partition re-assignment will
be triggered whenever the members of the group change or the subscription
* of the members changes. This can occur when processes die, new process
instances are added or old instances come back to life after failure.
- * Partition re-assignments can also be triggered by changes affecting the
subscribed topics (e.g. when the number of partitions is
+ * Partition re-assignments can also be triggered by changing affecting the
subscribed topics (e.g. when the number of partitions is
* administratively adjusted).
* <p>
* There are many uses for this functionality. One common use is saving
offsets in a custom store. By saving offsets in
* the {@link #onPartitionsRevoked(Collection)} call we can ensure that any
time partition assignment changes
* the offset gets saved.
* <p>
* Another use is flushing out any kind of cache of intermediate results the
consumer may be keeping. For example,
- * consider a case where the consumer is subscribed to a topic containing user
page views, and the goal is to count the
- * number of page views per user for each five minute window. Let's say the
topic is partitioned by the user id so that
+ * consider a case where the consumer is subscribing to a topic containing
user page views, and the goal is to count the
+ * number of page views per user for each five-minute window. Let's say the
topic is partitioned by the user id so that
* all events for a particular user go to a single consumer instance. The
consumer can keep in memory a running
- * tally of actions per user and only flush these out to a remote data store
when its cache gets too big. However if a
- * partition is reassigned it may want to automatically trigger a flush of
this cache, before the new owner takes over
+ * tally of actions per user and only flush these out to a remote data store
when its cache gets too big. However, if a
+ * partition is reassigned, it may want to automatically trigger a flush of
this cache, before the new owner takes over
Review Comment:
I think the additional commas are a nice improvement to readability, but I
would remove the existing one in "cache, before the new owner".
##########
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerRebalanceListener.java:
##########
@@ -28,37 +28,37 @@
* This is applicable when the consumer is having Kafka auto-manage group
membership. If the consumer directly assigns partitions,
* those partitions will never be reassigned and this callback is not
applicable.
* <p>
- * When Kafka is managing the group membership, a partition re-assignment will
be triggered any time the members of the group change or the subscription
+ * When Kafka is managing the group membership, a partition re-assignment will
be triggered whenever the members of the group change or the subscription
* of the members changes. This can occur when processes die, new process
instances are added or old instances come back to life after failure.
- * Partition re-assignments can also be triggered by changes affecting the
subscribed topics (e.g. when the number of partitions is
+ * Partition re-assignments can also be triggered by changing affecting the
subscribed topics (e.g. when the number of partitions is
Review Comment:
I don't think either of these changes is better than the existing "changes
affecting the subscribed topics".
--
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]
