You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/12/10 23:32:52 UTC
[GitHub] [kafka] lbradstreet opened a new pull request #9729: KAFKA-10839: improve consumer group coordinator unavailable message
lbradstreet opened a new pull request #9729:
URL: https://github.com/apache/kafka/pull/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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #9729: KAFKA-10839: improve consumer group coordinator unavailable message
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #9729:
URL: https://github.com/apache/kafka/pull/9729#discussion_r541218024
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java
##########
@@ -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");
Review comment:
Seems ok. Maybe "without receiving a successful heartbeat response"?
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java
##########
@@ -890,13 +890,20 @@ private synchronized Node coordinator() {
return this.coordinator;
}
- protected synchronized void markCoordinatorUnknown() {
- markCoordinatorUnknown(false);
+
+ protected synchronized void markCoordinatorUnknown(Errors error) {
+ markCoordinatorUnknown(false, "error response {}" + error.message());
Review comment:
Wonder if we can use the name of the error. Some of the messages will read awkwardly in the log message
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java
##########
@@ -890,13 +890,20 @@ private synchronized Node coordinator() {
return this.coordinator;
}
- protected synchronized void markCoordinatorUnknown() {
- markCoordinatorUnknown(false);
+
+ protected synchronized void markCoordinatorUnknown(Errors error) {
+ markCoordinatorUnknown(false, "error response {}" + error.message());
+ }
+
+ protected synchronized void markCoordinatorUnknown(String cause) {
+ markCoordinatorUnknown(false, cause);
}
- protected synchronized void markCoordinatorUnknown(boolean isDisconnected) {
+ protected synchronized void markCoordinatorUnknown(boolean isDisconnected, String cause) {
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,
Review comment:
nit: "Rediscovery will **be** attempted"
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lbradstreet commented on a change in pull request #9729: KAFKA-10839: improve consumer group coordinator unavailable message
Posted by GitBox <gi...@apache.org>.
lbradstreet commented on a change in pull request #9729:
URL: https://github.com/apache/kafka/pull/9729#discussion_r541424869
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java
##########
@@ -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");
Review comment:
Done
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java
##########
@@ -890,13 +890,20 @@ private synchronized Node coordinator() {
return this.coordinator;
}
- protected synchronized void markCoordinatorUnknown() {
- markCoordinatorUnknown(false);
+
+ protected synchronized void markCoordinatorUnknown(Errors error) {
+ markCoordinatorUnknown(false, "error response {}" + error.message());
Review comment:
Done
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java
##########
@@ -890,13 +890,20 @@ private synchronized Node coordinator() {
return this.coordinator;
}
- protected synchronized void markCoordinatorUnknown() {
- markCoordinatorUnknown(false);
+
+ protected synchronized void markCoordinatorUnknown(Errors error) {
+ markCoordinatorUnknown(false, "error response {}" + error.message());
+ }
+
+ protected synchronized void markCoordinatorUnknown(String cause) {
+ markCoordinatorUnknown(false, cause);
}
- protected synchronized void markCoordinatorUnknown(boolean isDisconnected) {
+ protected synchronized void markCoordinatorUnknown(boolean isDisconnected, String cause) {
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,
Review comment:
done
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji commented on a change in pull request #9729: KAFKA-10839: improve consumer group coordinator unavailable message
Posted by GitBox <gi...@apache.org>.
hachikuji commented on a change in pull request #9729:
URL: https://github.com/apache/kafka/pull/9729#discussion_r541213215
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java
##########
@@ -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");
Review comment:
I'm ok with it. This is just informational and the literal value is easier to read.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] hachikuji merged pull request #9729: KAFKA-10839: improve consumer group coordinator unavailable message
Posted by GitBox <gi...@apache.org>.
hachikuji merged pull request #9729:
URL: https://github.com/apache/kafka/pull/9729
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lbradstreet commented on a change in pull request #9729: KAFKA-10839: improve consumer group coordinator unavailable message
Posted by GitBox <gi...@apache.org>.
lbradstreet commented on a change in pull request #9729:
URL: https://github.com/apache/kafka/pull/9729#discussion_r541400985
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java
##########
@@ -890,13 +890,20 @@ private synchronized Node coordinator() {
return this.coordinator;
}
- protected synchronized void markCoordinatorUnknown() {
- markCoordinatorUnknown(false);
+
+ protected synchronized void markCoordinatorUnknown(Errors error) {
+ markCoordinatorUnknown(false, "error response {}" + error.message());
Review comment:
Oops, that was what I was trying to do.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lbradstreet commented on a change in pull request #9729: KAFKA-10839: improve consumer group coordinator unavailable message
Posted by GitBox <gi...@apache.org>.
lbradstreet commented on a change in pull request #9729:
URL: https://github.com/apache/kafka/pull/9729#discussion_r541425344
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/ConsumerCoordinator.java
##########
@@ -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());
Review comment:
Needs fixing
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lbradstreet commented on a change in pull request #9729: KAFKA-10839: improve consumer group coordinator unavailable message
Posted by GitBox <gi...@apache.org>.
lbradstreet commented on a change in pull request #9729:
URL: https://github.com/apache/kafka/pull/9729#discussion_r540578598
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java
##########
@@ -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");
Review comment:
Should this be extracted into a constant?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] lbradstreet commented on a change in pull request #9729: KAFKA-10839: improve consumer group coordinator unavailable message
Posted by GitBox <gi...@apache.org>.
lbradstreet commented on a change in pull request #9729:
URL: https://github.com/apache/kafka/pull/9729#discussion_r540578510
##########
File path: clients/src/main/java/org/apache/kafka/clients/consumer/internals/AbstractCoordinator.java
##########
@@ -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");
Review comment:
These messages feel a bit smelly
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org