MINOR : Add missing error code in ConsumerHeartbeatRequestManagerTest. #19024
Conversation
github-actions
bot
added
triage
There was a problem hiding this comment.
Thanks @ShivsundarR ! Just one nit.
Related thought: there is a very similar test in ShareMembershipMgr (testHeartbeatResponseOnErrorHandling), but instead of duplicating this check there, I wonder if that test makes sense on it's own or is more a duplicate that should be removed? The logic to handle responses is not specific to the share mgr (it's defined in the AbstractMembershipMgrTest), so isn't the test in ConsumerHeartbeatRequestManagerTest enough?
| @@ -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)); | |||
There was a problem hiding this 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)
There was a problem hiding this 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?
There was a problem hiding this 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
There was a problem hiding this comment.
Okay yeah that makes sense. Thank you.
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.
Updated javadocs for
ConsumerHeartbeatResponseandConsumerDescribeResponse.