You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "AndrewJSchofield (via GitHub)" <gi...@apache.org> on 2026/03/20 08:20:12 UTC
[PR] MINOR: Consumer tidying [kafka]
AndrewJSchofield opened a new pull request, #21832:
URL: https://github.com/apache/kafka/pull/21832
Resolves a couple of review comments from https://github.com/apache/kafka/pull/21711. The remaining comments are in `ShareConsumerTest` and will be addressed separately.
--
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: Consumer tidying [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #21832:
URL: https://github.com/apache/kafka/pull/21832#discussion_r2966399472
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ShareConsumerImpl.java:
##########
@@ -958,7 +959,7 @@ public Optional<Integer> acquisitionLockTimeoutMs() {
*/
@Override
public Map<MetricName, ? extends Metric> metrics() {
- return Map.copyOf(metrics.metrics());
+ return Collections.unmodifiableMap(metrics.metrics());
Review Comment:
done. https://github.com/apache/kafka/commit/754b347a5bbcd077d87f81dee0c29d86c70c478d
--
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: Consumer tidying [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield merged PR #21832:
URL: https://github.com/apache/kafka/pull/21832
--
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: Consumer tidying [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #21832:
URL: https://github.com/apache/kafka/pull/21832#discussion_r2965194139
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java:
##########
@@ -1832,7 +1832,7 @@ public Set<TopicPartition> assignment() {
public Set<String> subscription() {
acquireAndEnsureOpen();
try {
- return Collections.unmodifiableSet(subscriptions.subscription());
+ return Set.copyOf(subscriptions.subscription());
Review Comment:
I had opened https://issues.apache.org/jira/browse/KAFKA-20341 to update the docs, but it is totally fine to address them in this patch
Btw, we could update the docs of metrics as well.
--
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: Consumer tidying [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on code in PR #21832:
URL: https://github.com/apache/kafka/pull/21832#discussion_r2965310312
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java:
##########
@@ -1832,7 +1832,7 @@ public Set<TopicPartition> assignment() {
public Set<String> subscription() {
acquireAndEnsureOpen();
try {
- return Collections.unmodifiableSet(subscriptions.subscription());
+ return Set.copyOf(subscriptions.subscription());
Review Comment:
Let's do the docs updates the separate issue. I'll fix the grammatical error in this javadoc (internal class) in this PR.
--
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: Consumer tidying [kafka]
Posted by "viktorsomogyi (via GitHub)" <gi...@apache.org>.
viktorsomogyi commented on code in PR #21832:
URL: https://github.com/apache/kafka/pull/21832#discussion_r2964694638
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java:
##########
@@ -1832,7 +1832,7 @@ public Set<TopicPartition> assignment() {
public Set<String> subscription() {
acquireAndEnsureOpen();
try {
- return Collections.unmodifiableSet(subscriptions.subscription());
+ return Set.copyOf(subscriptions.subscription());
Review Comment:
Do you think callers usually won't expect the live view here that is the current behavior?
--
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: Consumer tidying [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #21832:
URL: https://github.com/apache/kafka/pull/21832#discussion_r2965897912
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/ShareConsumerImpl.java:
##########
@@ -958,7 +959,7 @@ public Optional<Integer> acquisitionLockTimeoutMs() {
*/
@Override
public Map<MetricName, ? extends Metric> metrics() {
- return Map.copyOf(metrics.metrics());
+ return Collections.unmodifiableMap(metrics.metrics());
Review Comment:
I'll backport this patch to the 4.3 branch, as this change ensures consistency in our logic.
--
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: Consumer tidying [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on code in PR #21832:
URL: https://github.com/apache/kafka/pull/21832#discussion_r2964957369
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java:
##########
@@ -1832,7 +1832,7 @@ public Set<TopicPartition> assignment() {
public Set<String> subscription() {
acquireAndEnsureOpen();
try {
- return Collections.unmodifiableSet(subscriptions.subscription());
+ return Set.copyOf(subscriptions.subscription());
Review Comment:
@lianetm What's your view here, given that you're all over the subscription state transitions and visibility at the moment?
--
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: Consumer tidying [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on code in PR #21832:
URL: https://github.com/apache/kafka/pull/21832#discussion_r2965095043
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java:
##########
@@ -1832,7 +1832,7 @@ public Set<TopicPartition> assignment() {
public Set<String> subscription() {
acquireAndEnsureOpen();
try {
- return Collections.unmodifiableSet(subscriptions.subscription());
+ return Set.copyOf(subscriptions.subscription());
Review Comment:
I would say we should return a copy, to keep the behaviour consistent with the ClassicConsumer (otherwise applications wrongly reusing old calls of this across assign/subscribe will behave differently for classic/async).
Since we're here, could we fix the java doc please? (". or")
--
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: Consumer tidying [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #21832:
URL: https://github.com/apache/kafka/pull/21832#discussion_r2965667355
##########
clients/src/main/java/org/apache/kafka/clients/consumer/internals/AsyncKafkaConsumer.java:
##########
@@ -1832,7 +1832,7 @@ public Set<TopicPartition> assignment() {
public Set<String> subscription() {
acquireAndEnsureOpen();
try {
- return Collections.unmodifiableSet(subscriptions.subscription());
+ return Set.copyOf(subscriptions.subscription());
Review Comment:
> Let's do the docs updates the separate issue. I'll fix the grammatical error in this javadoc (internal class) in this PR.
I've added a comment to KAFKA-20341 to update the metrics documentation as well.
--
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