KAFKA-10839: improve consumer group coordinator unavailable message #9729
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lbradstreet
commented
Dec 10, 2020
| @@ -1355,7 +1362,7 @@ public void run() { | |||
| } else if (heartbeat.sessionTimeoutExpired(now)) { | |||
| // the session timeout has expired without seeing a successful heartbeat, so we should | |||
| // probably make sure the coordinator is still healthy. | |||
| markCoordinatorUnknown(); | |||
| markCoordinatorUnknown("session timed out without heartbeat"); | |||
There was a problem hiding this comment.
These messages feel a bit smelly
There was a problem hiding this comment.
Seems ok. Maybe "without receiving a successful heartbeat response"?
lbradstreet
commented
Dec 10, 2020
| @@ -257,7 +257,7 @@ protected synchronized boolean ensureCoordinatorReady(final Timer timer) { | |||
| } else if (coordinator != null && client.isUnavailable(coordinator)) { | |||
| // we found the coordinator, but the connection has failed, so mark | |||
| // it dead and backoff before retrying discovery | |||
| markCoordinatorUnknown(); | |||
| markCoordinatorUnknown("coordinator unavailable"); | |||
There was a problem hiding this comment.
Should this be extracted into a constant?
There was a problem hiding this comment.
I'm ok with it. This is just informational and the literal value is easier to read.
hachikuji
reviewed
Dec 11, 2020
| markCoordinatorUnknown(false); | ||
|
|
||
| protected synchronized void markCoordinatorUnknown(Errors error) { | ||
| markCoordinatorUnknown(false, "error response {}" + error.message()); |
There was a problem hiding this comment.
Wonder if we can use the name of the error. Some of the messages will read awkwardly in the log message
There was a problem hiding this comment.
Oops, that was what I was trying to do.
| if (this.coordinator != null) { | ||
| log.info("Group coordinator {} is unavailable or invalid, will attempt rediscovery", this.coordinator); | ||
| log.info("Group coordinator {} is unavailable or invalid due to cause: {}." | ||
| + "isDisconnected: {}. Rediscovery will attempted.", this.coordinator, |
There was a problem hiding this comment.
nit: "Rediscovery will be attempted"
| @@ -1355,7 +1362,7 @@ public void run() { | |||
| } else if (heartbeat.sessionTimeoutExpired(now)) { | |||
| // the session timeout has expired without seeing a successful heartbeat, so we should | |||
| // probably make sure the coordinator is still healthy. | |||
| markCoordinatorUnknown(); | |||
| markCoordinatorUnknown("session timed out without heartbeat"); | |||
There was a problem hiding this comment.
Seems ok. Maybe "without receiving a successful heartbeat response"?
lbradstreet
commented
Dec 11, 2020
| @@ -1311,7 +1311,7 @@ public void handle(OffsetFetchResponse response, RequestFuture<Map<TopicPartitio | |||
| future.raise(error); | |||
| } else if (error == Errors.NOT_COORDINATOR) { | |||
| // re-discover the coordinator and retry | |||
| markCoordinatorUnknown(); | |||
| markCoordinatorUnknown(error.message()); | |||
hachikuji
approved these changes
Dec 15, 2020
3 tasks
hachikuji
added a commit
that referenced
this pull request
Dec 15, 2020
Missed this in #9729. The substitution in `markCoordinatorUnknown` does not work because the argument is not provided as a parameter. Reviewers: Ismael Juma <ismael@juma.me.uk>
hachikuji
pushed a commit
that referenced
this pull request
Dec 15, 2020
…9729) When a consumer encounters an issue that triggers marking it to mark coordinator as unknown, the error message it prints does not give much context about the error that triggered it. This change includes the response error that triggered the transition or any other cause if not triggered by an error code in a response. Reviewers: Jason Gustafson <jason@confluent.io>
hachikuji
added a commit
that referenced
this pull request
Dec 15, 2020
Missed this in #9729. The substitution in `markCoordinatorUnknown` does not work because the argument is not provided as a parameter. Reviewers: Ismael Juma <ismael@juma.me.uk>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When a consumer encounters an issue that triggers marking it to mark coordinator as unknown, the error message it prints does not give much context about the error that triggered it. This change includes the response error that triggered the transition or any other cause if not triggered by an error code in a response.