You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "ShivsundarR (via GitHub)" <gi...@apache.org> on 2025/02/25 08:54:36 UTC
[PR] MINOR : Add missing error code in ConsumerHeartbeatRequestManagerTest. [kafka]
ShivsundarR opened a new pull request, #19024:
URL: https://github.com/apache/kafka/pull/19024
***What***
PR fixes a minor bug in https://github.com/apache/kafka/pull/18986/files#diff-9fbddcb7950c03d236f3791df42fe4de0feb6d4ff25c7d8bec80545ff9378af3R585.
The test was previously not taking in the intented error code (TOPIC_AUTHORIZATION_FAILED), now it would.
--
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 : Add missing error code in ConsumerHeartbeatRequestManagerTest. [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on code in PR #19024:
URL: https://github.com/apache/kafka/pull/19024#discussion_r1970153512
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerHeartbeatRequestManagerTest.java:
##########
@@ -1029,7 +1029,8 @@ private static Collection<Arguments> errorProvider() {
Arguments.of(Errors.UNSUPPORTED_VERSION, true),
Arguments.of(Errors.UNRELEASED_INSTANCE_ID, true),
Arguments.of(Errors.FENCED_INSTANCE_ID, true),
- Arguments.of(Errors.GROUP_MAX_SIZE_REACHED, true));
+ Arguments.of(Errors.GROUP_MAX_SIZE_REACHED, true),
+ Arguments.of(Errors.TOPIC_AUTHORIZATION_FAILED, true));
Review Comment:
> the ShareHeartbeatRequestManagerTest, yes they are identical, except there is one check missing
Exactly, that shows how the Share test has already fallen behind, it is redundant and we're not maintaining it. We should review and remove it imo (but can be done separately, I just noticed it here)
>if we decide to handle heartbeat responses differently for regular consumers and share consumers, it will be better to have 2 different
yes, but better to add it when needed (and not have a duplicate now that we either maintain or leave behind)
I will file a jira and we can consider the improvement separately.
-- update
https://issues.apache.org/jira/browse/KAFKA-18862
--
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 : Add missing error code in ConsumerHeartbeatRequestManagerTest. [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on code in PR #19024:
URL: https://github.com/apache/kafka/pull/19024#discussion_r1969823489
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerHeartbeatRequestManagerTest.java:
##########
@@ -1029,7 +1029,8 @@ private static Collection<Arguments> errorProvider() {
Arguments.of(Errors.UNSUPPORTED_VERSION, true),
Arguments.of(Errors.UNRELEASED_INSTANCE_ID, true),
Arguments.of(Errors.FENCED_INSTANCE_ID, true),
- Arguments.of(Errors.GROUP_MAX_SIZE_REACHED, true));
+ Arguments.of(Errors.GROUP_MAX_SIZE_REACHED, true),
+ Arguments.of(Errors.TOPIC_AUTHORIZATION_FAILED, true));
Review Comment:
nit: even thought it's a non-retriable exception, this is not considered fatal in the client, so we should pass false here. (It doesn't affect the test now because the isFatal param is not used in this specific case, but just to make it right in case we change the test in the future)
--
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 : Add missing error code in ConsumerHeartbeatRequestManagerTest. [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on code in PR #19024:
URL: https://github.com/apache/kafka/pull/19024#discussion_r1970153512
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerHeartbeatRequestManagerTest.java:
##########
@@ -1029,7 +1029,8 @@ private static Collection<Arguments> errorProvider() {
Arguments.of(Errors.UNSUPPORTED_VERSION, true),
Arguments.of(Errors.UNRELEASED_INSTANCE_ID, true),
Arguments.of(Errors.FENCED_INSTANCE_ID, true),
- Arguments.of(Errors.GROUP_MAX_SIZE_REACHED, true));
+ Arguments.of(Errors.GROUP_MAX_SIZE_REACHED, true),
+ Arguments.of(Errors.TOPIC_AUTHORIZATION_FAILED, true));
Review Comment:
> the ShareHeartbeatRequestManagerTest, yes they are identical, except there is one check missing
Exactly, that shows how the Share test has already fallen behind, it is redundant and we're not maintaining it. We should review and remove it imo (but can be done separately, I just noticed it here)
>if we decide to handle heartbeat responses differently for regular consumers and share consumers, it will be better to have 2 different
yes, but better to add it when needed (and not have a duplicate now that have to maintain or leave behind)
I will file a jira and we can consider the improvement 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 : Add missing error code in ConsumerHeartbeatRequestManagerTest. [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on code in PR #19024:
URL: https://github.com/apache/kafka/pull/19024#discussion_r1970153512
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerHeartbeatRequestManagerTest.java:
##########
@@ -1029,7 +1029,8 @@ private static Collection<Arguments> errorProvider() {
Arguments.of(Errors.UNSUPPORTED_VERSION, true),
Arguments.of(Errors.UNRELEASED_INSTANCE_ID, true),
Arguments.of(Errors.FENCED_INSTANCE_ID, true),
- Arguments.of(Errors.GROUP_MAX_SIZE_REACHED, true));
+ Arguments.of(Errors.GROUP_MAX_SIZE_REACHED, true),
+ Arguments.of(Errors.TOPIC_AUTHORIZATION_FAILED, true));
Review Comment:
> the ShareHeartbeatRequestManagerTest, yes they are identical, except there is one check missing
Exactly, that shows how the Share test has already fallen behind, it is redundant and we're not maintaining it. We should review and remove it imo (but can be done separately, I just noticed it here)
>if we decide to handle heartbeat responses differently for regular consumers and share consumers, it will be better to have 2 different
yes, but better to add it when needed (and not have a duplicate now that we either maintain or leave behind)
I will file a jira and we can consider the improvement separately.
-- update
https://issues.apache.org/jira/browse/KAFKA-18862
--
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 : Add missing error code in ConsumerHeartbeatRequestManagerTest. [kafka]
Posted by "ShivsundarR (via GitHub)" <gi...@apache.org>.
ShivsundarR commented on code in PR #19024:
URL: https://github.com/apache/kafka/pull/19024#discussion_r1970009792
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerHeartbeatRequestManagerTest.java:
##########
@@ -1029,7 +1029,8 @@ private static Collection<Arguments> errorProvider() {
Arguments.of(Errors.UNSUPPORTED_VERSION, true),
Arguments.of(Errors.UNRELEASED_INSTANCE_ID, true),
Arguments.of(Errors.FENCED_INSTANCE_ID, true),
- Arguments.of(Errors.GROUP_MAX_SIZE_REACHED, true));
+ Arguments.of(Errors.GROUP_MAX_SIZE_REACHED, true),
+ Arguments.of(Errors.TOPIC_AUTHORIZATION_FAILED, true));
Review Comment:
Thanks, okay yeah I will change it to false, makes sense.
Also for the `ShareHeartbeatRequestManagerTest`, yes they are identical, except there is one check missing in that test which is present in `ConsumerHeartbeatRequestManagerTest`.
```
case FENCED_MEMBER_EPOCH:
verify(backgroundEventHandler, never()).add(any());
assertNextHeartbeatTiming(0);
break;
```
Also in future if we decide to handle heartbeat responses differently for regular consumers and share consumers, it will be better to have 2 different tests right?
--
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 : Add missing error code in ConsumerHeartbeatRequestManagerTest. [kafka]
Posted by "ShivsundarR (via GitHub)" <gi...@apache.org>.
ShivsundarR commented on code in PR #19024:
URL: https://github.com/apache/kafka/pull/19024#discussion_r1970009792
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerHeartbeatRequestManagerTest.java:
##########
@@ -1029,7 +1029,8 @@ private static Collection<Arguments> errorProvider() {
Arguments.of(Errors.UNSUPPORTED_VERSION, true),
Arguments.of(Errors.UNRELEASED_INSTANCE_ID, true),
Arguments.of(Errors.FENCED_INSTANCE_ID, true),
- Arguments.of(Errors.GROUP_MAX_SIZE_REACHED, true));
+ Arguments.of(Errors.GROUP_MAX_SIZE_REACHED, true),
+ Arguments.of(Errors.TOPIC_AUTHORIZATION_FAILED, true));
Review Comment:
Thanks, okay yeah I will change it to false, makes sense.
Also for the ShareHeartbeatRequestManagerTest, yes they are identical, except there is one check missing in that test which is present in ConsumerHeartbeatRequestManagerTest.
```
case FENCED_MEMBER_EPOCH:
verify(backgroundEventHandler, never()).add(any());
assertNextHeartbeatTiming(0);
break;
```
Also in future if we decide to handle heartbeat responses differently for regular consumers and share consumers, it will be better to have 2 different tests right?
--
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 : Add missing error code in ConsumerHeartbeatRequestManagerTest. [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm merged PR #19024:
URL: https://github.com/apache/kafka/pull/19024
--
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 : Add missing error code in ConsumerHeartbeatRequestManagerTest. [kafka]
Posted by "lianetm (via GitHub)" <gi...@apache.org>.
lianetm commented on code in PR #19024:
URL: https://github.com/apache/kafka/pull/19024#discussion_r1970153512
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerHeartbeatRequestManagerTest.java:
##########
@@ -1029,7 +1029,8 @@ private static Collection<Arguments> errorProvider() {
Arguments.of(Errors.UNSUPPORTED_VERSION, true),
Arguments.of(Errors.UNRELEASED_INSTANCE_ID, true),
Arguments.of(Errors.FENCED_INSTANCE_ID, true),
- Arguments.of(Errors.GROUP_MAX_SIZE_REACHED, true));
+ Arguments.of(Errors.GROUP_MAX_SIZE_REACHED, true),
+ Arguments.of(Errors.TOPIC_AUTHORIZATION_FAILED, true));
Review Comment:
> the ShareHeartbeatRequestManagerTest, yes they are identical, except there is one check missing
Exactly, that shows how the Share test has already fallen behind, it is redundant and we're not maintaining it. We should review and remove it imo (but can be done separately, I just noticed it here)
>if we decide to handle heartbeat responses differently for regular consumers and share consumers, it will be better to have 2 different
yes, but better to add it when needed (and not have a duplicate now that have to maintain or leave behind)
I will file a jira and we can consider the improvement 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 : Add missing error code in ConsumerHeartbeatRequestManagerTest. [kafka]
Posted by "ShivsundarR (via GitHub)" <gi...@apache.org>.
ShivsundarR commented on code in PR #19024:
URL: https://github.com/apache/kafka/pull/19024#discussion_r1970190064
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerHeartbeatRequestManagerTest.java:
##########
@@ -1029,7 +1029,8 @@ private static Collection<Arguments> errorProvider() {
Arguments.of(Errors.UNSUPPORTED_VERSION, true),
Arguments.of(Errors.UNRELEASED_INSTANCE_ID, true),
Arguments.of(Errors.FENCED_INSTANCE_ID, true),
- Arguments.of(Errors.GROUP_MAX_SIZE_REACHED, true));
+ Arguments.of(Errors.GROUP_MAX_SIZE_REACHED, true),
+ Arguments.of(Errors.TOPIC_AUTHORIZATION_FAILED, true));
Review Comment:
Okay thanks, yeah that makes sense. Thank you.
##########
clients/src/test/java/org/apache/kafka/clients/consumer/internals/ConsumerHeartbeatRequestManagerTest.java:
##########
@@ -1029,7 +1029,8 @@ private static Collection<Arguments> errorProvider() {
Arguments.of(Errors.UNSUPPORTED_VERSION, true),
Arguments.of(Errors.UNRELEASED_INSTANCE_ID, true),
Arguments.of(Errors.FENCED_INSTANCE_ID, true),
- Arguments.of(Errors.GROUP_MAX_SIZE_REACHED, true));
+ Arguments.of(Errors.GROUP_MAX_SIZE_REACHED, true),
+ Arguments.of(Errors.TOPIC_AUTHORIZATION_FAILED, true));
Review Comment:
Okay yeah that makes sense. Thank you.
--
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