You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "TaiJuWu (via GitHub)" <gi...@apache.org> on 2024/05/26 07:37:01 UTC
[PR] MINOR:Modify ConsumerRebalanceListener doc [kafka]
TaiJuWu opened a new pull request, #16083:
URL: https://github.com/apache/kafka/pull/16083
*More detailed description of your change,
Only document
*Summary of testing strategy (including rationale)
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR:Improve ConsumerRebalanceListener doc [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on PR #16083:
URL: https://github.com/apache/kafka/pull/16083#issuecomment-2139394200
Hi @AndrewJSchofield @lianetm
Thanks for your review and I address your comments, please review again.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR:Improve ConsumerRebalanceListener doc [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on code in PR #16083:
URL: https://github.com/apache/kafka/pull/16083#discussion_r1616090879
##########
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 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 minutes 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
* consumption.
* <p>
* This callback will only execute in the user thread as part of the {@link Consumer#poll(java.time.Duration) poll(long)} call
* whenever partition assignment changes.
* <p>
* Under normal conditions, if a partition is reassigned from one consumer to another, then the old consumer will
* always invoke {@link #onPartitionsRevoked(Collection) onPartitionsRevoked} for that partition prior to the new consumer
- * invoking {@link #onPartitionsAssigned(Collection) onPartitionsAssigned} for the same partition. So if offsets or other state is saved in the
- * {@link #onPartitionsRevoked(Collection) onPartitionsRevoked} call by one consumer member, it will be always accessible by the time the
+ * invoking {@link #onPartitionsAssigned(Collection) onPartitionsAssigned} for the same partition. So if offsets or other states are saved in the
+ * {@link #onPartitionsRevoked(Collection) onPartitionsRevoked} call by one consumer member, it will be always accessible by the time, the
* other consumer member taking over that partition and triggering its {@link #onPartitionsAssigned(Collection) onPartitionsAssigned} callback to load the state.
* <p>
- * You can think of revocation as a graceful way to give up ownership of a partition. In some cases, the consumer may not have an opportunity to do so.
+ * You can think of revocation as a graceful way to give up ownership of a partition. In some cases, a consumer may not have an opportunity to do so.
Review Comment:
Both seems fine, but I don't see we're fixing an issue with the sentence, and actually "the consumer" is how it's used throughout the whole java doc, so why changing it? I would leave it how it was to be consistent.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR:Improve ConsumerRebalanceListener doc [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu closed pull request #16083: MINOR:Improve ConsumerRebalanceListener doc
URL: https://github.com/apache/kafka/pull/16083
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR:Improve ConsumerRebalanceListener doc [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on code in PR #16083:
URL: https://github.com/apache/kafka/pull/16083#discussion_r1616124179
##########
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 which users can implement to provide handling of offset commits for a customized store.
Review Comment:
uhm here I find the change is not really making things better. I wouldn't necessarily change this, but if we think it should be improved maybe just `A callback method the user can implement to provide handling of offset commits **sent to** a customized store`
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR:Improve ConsumerRebalanceListener doc [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR:Improve ConsumerRebalanceListener doc [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on code in PR #16083:
URL: https://github.com/apache/kafka/pull/16083#discussion_r1616082089
##########
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 the subscribed topics (e.g. when the number of partitions is
Review Comment:
I guess it's about interpretation, but I actually find the original version much clearer. "changing the subscribed topics" seems a bit confusing, as its what we would use to refer to "changing the subscribed topic from T1 to T2", but here we want to refer to changes affecting the topics (ex. partition count changes)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR:Improve ConsumerRebalanceListener doc [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #16083:
URL: https://github.com/apache/kafka/pull/16083#discussion_r1615204659
##########
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:
how about `by changing 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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR:Improve ConsumerRebalanceListener doc [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on code in PR #16083:
URL: https://github.com/apache/kafka/pull/16083#discussion_r1617261952
##########
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:
Agree with @AndrewJSchofield that it would be an improvement to start stating the general purpose of the callback (cleaning up resources being reassigned.. as Andrew described). Still, I would also keep the offset management example, seems a simple one to understand how to use the callback. I honestly don't know how often it's used out there, but it's one of the [Usage examples](https://github.com/apache/kafka/blob/699438b7f7801115cb7d66dba202b25af7ff8a1f/clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java#L350) described in the `KafkaConsumer` documentation, so from a user's perspective it would align nicely to refer to the same example here I would say.....side note: I may be a bit biased here, I've been recently working on this use case of the callbacks on the new consumer :)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR:Improve ConsumerRebalanceListener doc [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on PR #16083:
URL: https://github.com/apache/kafka/pull/16083#issuecomment-2319304919
> Thanks for the PR @TaiJuWu!
>
> The changes appear to be grammatical, and the bulk of them are an improvement. I don't see how it can hurt to be a little more precise, where possible.
>
> Thanks!
Hi @kirktrue , update according your comments, PTAL.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR:Improve ConsumerRebalanceListener doc [kafka]
Posted by "kirktrue (via GitHub)" <gi...@apache.org>.
kirktrue commented on code in PR #16083:
URL: https://github.com/apache/kafka/pull/16083#discussion_r1735416616
##########
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerRebalanceListener.java:
##########
@@ -144,9 +144,9 @@ public interface ConsumerRebalanceListener {
void onPartitionsRevoked(Collection<TopicPartition> partitions);
/**
- * A callback method the user can implement to provide handling of customized offsets on completion of a successful
- * partition re-assignment. This method will be called after the partition re-assignment completes and before the
- * consumer starts fetching data, and only as the result of a {@link Consumer#poll(java.time.Duration) poll(long)} call.
+ * A callback method the user can implement to be notified when partitions are assigned to this consumer.
Review Comment:
I think that "provide handling" is fine, especially since it was used for `onPartitionsRevoked()`.
```suggestion
* A callback method the user can implement to provide handling when partitions are assigned to this consumer.
```
##########
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerRebalanceListener.java:
##########
@@ -178,7 +178,7 @@ public interface ConsumerRebalanceListener {
* For example, this function is called if a consumer's session timeout has expired, or if a fatal error has been
* received indicating the consumer is no longer part of the group.
* <p>
- * By default it will just trigger {@link ConsumerRebalanceListener#onPartitionsRevoked}; for users who want to distinguish
+ * By default, it just triggers {@link ConsumerRebalanceListener#onPartitionsRevoked}; for users who want to distinguish
Review Comment:
I'm not a grammar expert, but what about:
```suggestion
* By default this function triggers {@link ConsumerRebalanceListener#onPartitionsRevoked}; for users who want to distinguish
```
##########
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:
I'm not sure that "sent to" adds that much.
Maybe:
```suggestion
* A callback method the user can implement to provide handling of offset commits **stored in** a customized store.
```
Or:
```suggestion
* A callback method the user can implement to provide handling of offset commits **managed in** a customized store.
```
##########
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.
* This method will be called during a rebalance operation when the consumer has to give up some partitions.
* It can also be called when consumer is being closed ({@link KafkaConsumer#close(Duration)})
* or is unsubscribing ({@link KafkaConsumer#unsubscribe()}).
* It is recommended that offsets should be committed in this callback to either Kafka or a
* custom offset store to prevent duplicate data.
* <p>
* In eager rebalancing, it will always be called at the start of a rebalance and after the consumer stops fetching data.
- * In cooperative rebalancing, it will be called at the end of a rebalance on the set of partitions being revoked iff the set is non-empty.
+ * In cooperative rebalancing, it will be called at the end of a rebalance on the set of partitions being revoked if and only if the set is non-empty.
Review Comment:
"iff" is an understood term, isn't it?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR:Improve ConsumerRebalanceListener doc [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on code in PR #16083:
URL: https://github.com/apache/kafka/pull/16083#discussion_r1616099696
##########
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 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 minutes window. Let's say the topic is partitioned by the user id so that
Review Comment:
"minute" should be fine here because it's part of a modifier for "window" ("five-minute window" if we want to be really accurate about it)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR:Improve ConsumerRebalanceListener doc [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #16083:
URL: https://github.com/apache/kafka/pull/16083#issuecomment-2169107085
@TaiJuWu any updates? I do love those docs improvement
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR:Improve ConsumerRebalanceListener doc [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #16083:
URL: https://github.com/apache/kafka/pull/16083#discussion_r1616331569
##########
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 the subscribed topics (e.g. when the number of partitions is
Review Comment:
Got it. Thanks for your explanation.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR:Improve ConsumerRebalanceListener doc [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on PR #16083:
URL: https://github.com/apache/kafka/pull/16083#issuecomment-2133955005
@lianetm Thanks for your comments, I have addressed your comment. Please review.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR:Improve ConsumerRebalanceListener doc [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on code in PR #16083:
URL: https://github.com/apache/kafka/pull/16083#discussion_r1620630034
##########
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:
"changing affecting" is not grammatical. I suggest reverting to "changes affecting the subscribed topics".
##########
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerRebalanceListener.java:
##########
@@ -144,8 +144,9 @@ public interface ConsumerRebalanceListener {
void onPartitionsRevoked(Collection<TopicPartition> partitions);
/**
- * A callback method the user can implement to provide handling of customized offsets on completion of a successful
- * partition re-assignment. This method will be called after the partition re-assignment completes and before the
+ * A callback method the user is providing handling of cleanup up resource which are being assigned to other customers and
Review Comment:
This one isn't right. It's not usually used for cleanup and "customers" is not right. I suggest "A callback method the user can implement to be notified when partitions are assigned to this consumer. This method will be called after the partition re-assignment completes and before....."
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR:Improve ConsumerRebalanceListener doc [kafka]
Posted by "TaiJuWu (via GitHub)" <gi...@apache.org>.
TaiJuWu commented on code in PR #16083:
URL: https://github.com/apache/kafka/pull/16083#discussion_r1615418058
##########
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:
Great for me. I will address your comments tonight.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR:Improve ConsumerRebalanceListener doc [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on code in PR #16083:
URL: https://github.com/apache/kafka/pull/16083#discussion_r1616077491
##########
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 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 minutes 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
* consumption.
* <p>
* This callback will only execute in the user thread as part of the {@link Consumer#poll(java.time.Duration) poll(long)} call
* whenever partition assignment changes.
* <p>
* Under normal conditions, if a partition is reassigned from one consumer to another, then the old consumer will
* always invoke {@link #onPartitionsRevoked(Collection) onPartitionsRevoked} for that partition prior to the new consumer
- * invoking {@link #onPartitionsAssigned(Collection) onPartitionsAssigned} for the same partition. So if offsets or other state is saved in the
- * {@link #onPartitionsRevoked(Collection) onPartitionsRevoked} call by one consumer member, it will be always accessible by the time the
+ * invoking {@link #onPartitionsAssigned(Collection) onPartitionsAssigned} for the same partition. So if offsets or other states are saved in the
+ * {@link #onPartitionsRevoked(Collection) onPartitionsRevoked} call by one consumer member, it will be always accessible by the time, the
Review Comment:
This added `times,` seems to be splitting the sentence at the wrong place I would say, and we still don't have the idea right as I see it. What about:
```suggestion
* {@link #onPartitionsRevoked(Collection) onPartitionsRevoked} call by one consumer member, it will be always accessible by the time the other consumer taking over that partition **gets a call to** its {@link #onPartitionsAssigned(Collection) onPartitionsAssigned} callback to load the state.
```
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org