Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MINOR: Do not print log4j for memberId required #9667

Conversation

guozhangwang
Copy link
Contributor

For MemberIdRequiredException, we would not print the exception at INFO with a full exception message since it may introduce more confusion that clearance.

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@guozhangwang
Copy link
Contributor Author

@hachikuji @abbccdda

Copy link
Contributor

@chia7712 chia7712 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guozhangwang Nice improvement. It makes sense to me to eliminate this "error message" as it is NOT fatal.

@@ -465,7 +465,11 @@ boolean joinGroupIfNeeded(final Timer timer) {
}
} else {
final RuntimeException exception = future.exception();
log.info("Rebalance failed.", exception);

if (!(exception instanceof MemberIdRequiredException)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is JoinGroupResponseHandler a better place to log error? For example, the error UNKNOWN_MEMBER_ID is log twice.
(https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java#L605)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For another, does it need some comment to explain why MemberIdRequiredException is excluded.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I agree with you: it is a bit redundant. I think originally it is because this future may contain the error that is either from join-response or from sync-response, but at the moment both errors are logged inside the Join/SyncGroupHandler anyways. Just to maintain compatibility in case some trouble shooting scenarios are dependent on this summary entry for now.

Copy link
Contributor

@abbccdda abbccdda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add an explanation here for why to skip the member.id required exception? (as it's transient)

@guozhangwang
Copy link
Contributor Author

Maybe add an explanation here for why to skip the member.id required exception? (as it's transient)

Ack, will do.

@guozhangwang guozhangwang merged commit a57486e into apache:trunk Dec 4, 2020
@guozhangwang guozhangwang deleted the KMinor-do-not-print-for-memberID-required branch December 4, 2020 22:02
mimaison pushed a commit that referenced this pull request Dec 10, 2020
For MemberIdRequiredException, we would not print the exception at INFO with a full exception message since it may introduce more confusion that clearance.

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Boyang Chen <boyang@confluent.io>
bbejeck pushed a commit that referenced this pull request Dec 10, 2020
For MemberIdRequiredException, we would not print the exception at INFO with a full exception message since it may introduce more confusion that clearance.

Reviewers: Chia-Ping Tsai <chia7712@gmail.com>, Boyang Chen <boyang@confluent.io>
@bbejeck
Copy link
Contributor

bbejeck commented Dec 10, 2020

cherry-picked to 2.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants