You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "chia7712 (via GitHub)" <gi...@apache.org> on 2023/05/02 19:11:15 UTC
[GitHub] [kafka] chia7712 opened a new pull request, #13659: MINOR: add docs to remind reader that impl of ConsumerPartitionAssign…
chia7712 opened a new pull request, #13659:
URL: https://github.com/apache/kafka/pull/13659
https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerPartitionAssignor.java#L306
The impl of `ConsumerPartitionAssignor` can get configs from Consumer if it implements the `Configurable` interface. That is a useful feature but there is no related docs.
Maybe the better change is to make `ConsumerPartitionAssignor` extend `Configurable` directly. However, I'd like to make "small" patch first :)
### 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
[GitHub] [kafka] chia7712 merged pull request #13659: MINOR: add docs to remind reader that impl of ConsumerPartitionAssign…
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #13659:
URL: https://github.com/apache/kafka/pull/13659
--
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
[GitHub] [kafka] chia7712 commented on pull request #13659: MINOR: add docs to remind reader that impl of ConsumerPartitionAssign…
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #13659:
URL: https://github.com/apache/kafka/pull/13659#issuecomment-1536128806
failed tests are unrelated. Will merge it later
--
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
[GitHub] [kafka] kirktrue commented on pull request #13659: MINOR: add docs to remind reader that impl of ConsumerPartitionAssign…
Posted by "kirktrue (via GitHub)" <gi...@apache.org>.
kirktrue commented on PR #13659:
URL: https://github.com/apache/kafka/pull/13659#issuecomment-1533285689
This looks good to me.
I looked briefly at the code that calls [`configure()`](https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerPartitionAssignor.java#L307). But I was a little surprised when I ran the [`ConsumerPartitionAssignorTest`](https://github.com/apache/kafka/blob/trunk/clients/src/test/java/org/apache/kafka/clients/consumer/ConsumerPartitionAssignorTest.java) with code coverage enabled and those particular tests aren't actually testing that functionality.
Would you be up for adding a test or two to `ConsumerPartitionAssignorTest` to make sure that functionality is properly covered? If not, I'm happy to add it myself.
Thanks!
--
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
[GitHub] [kafka] dajac commented on a diff in pull request #13659: MINOR: add docs to remind reader that impl of ConsumerPartitionAssign…
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #13659:
URL: https://github.com/apache/kafka/pull/13659#discussion_r1182960724
##########
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerPartitionAssignor.java:
##########
@@ -39,11 +39,13 @@
* as the group coordinator. The coordinator selects one member to perform the group assignment and
* propagates the subscriptions of all members to it. Then {@link #assign(Cluster, GroupSubscription)} is called
* to perform the assignment and the results are forwarded back to each respective members
- *
+ * <p>
* In some cases, it is useful to forward additional metadata to the assignor in order to make
* assignment decisions. For this, you can override {@link #subscriptionUserData(Set)} and provide custom
* userData in the returned Subscription. For example, to have a rack-aware assignor, an implementation
* can use this user data to forward the rackId belonging to each member.
+ * <p>
+ * the implementation can extend {@link Configurable} to get configs from consumer.
Review Comment:
nit: `The implementation...`
--
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
[GitHub] [kafka] chia7712 commented on pull request #13659: MINOR: add docs to remind reader that impl of ConsumerPartitionAssign…
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #13659:
URL: https://github.com/apache/kafka/pull/13659#issuecomment-1533287238
@kirktrue thanks for feedback. will address it later.
--
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
[GitHub] [kafka] chia7712 commented on pull request #13659: MINOR: add docs to remind reader that impl of ConsumerPartitionAssign…
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #13659:
URL: https://github.com/apache/kafka/pull/13659#issuecomment-1534435900
@kirktrue could you take a look?
--
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