Re: [PR] MINOR: consumer log fixes [kafka]
chia7712 merged PR #16345: URL: https://github.com/apache/kafka/pull/16345 -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: consumer log fixes [kafka]
kirktrue commented on code in PR #16345:
URL: https://github.com/apache/kafka/pull/16345#discussion_r1643453935
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java:
##
@@ -785,6 +805,13 @@ void removeRequest() {
}
}
+// Visible for testing
+Optional lastEpochSentOnCommit() {
+return lastEpochSentOnCommit;
+}
+
+
+
Review Comment:
Sorry for being the whitespace police 😆
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: consumer log fixes [kafka]
kirktrue commented on code in PR #16345:
URL: https://github.com/apache/kafka/pull/16345#discussion_r1643453077
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/MembershipManagerImpl.java:
##
@@ -362,7 +362,7 @@ private void transitionTo(MemberState nextState) {
metricsManager.recordRebalanceStarted(time.milliseconds());
}
-log.trace("Member {} with epoch {} transitioned from {} to {}.",
memberId, memberEpoch, state, nextState);
+log.info("Member {} with epoch {} transitioned from {} to {}.",
memberId, memberEpoch, state, nextState);
Review Comment:
OK, yes, that totally makes sense 👍 Plus, when community members file issues
against KIP-848 client work, it's better to have those logs included by
default. Good call 😄
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: consumer log fixes [kafka]
lianetm commented on code in PR #16345:
URL: https://github.com/apache/kafka/pull/16345#discussion_r1643448438
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/MembershipManagerImpl.java:
##
@@ -362,7 +362,7 @@ private void transitionTo(MemberState nextState) {
metricsManager.recordRebalanceStarted(time.milliseconds());
}
-log.trace("Member {} with epoch {} transitioned from {} to {}.",
memberId, memberEpoch, state, nextState);
+log.info("Member {} with epoch {} transitioned from {} to {}.",
memberId, memberEpoch, state, nextState);
Review Comment:
While troubleshooting different scenarios on the stress tests we're running,
we always end up finding ourselves struggling to understand the member state
just because we don't have this log info handy, so the intention was to move it
up at least on this Preview stage to easily track the state machine. This
should really only come out on events that we do care about (joining, leaving,
reconciling, errors), but if on practice we see it ends up generating more
noisy than the value it has we'll lower it down then. Makes sense?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: consumer log fixes [kafka]
lianetm commented on code in PR #16345:
URL: https://github.com/apache/kafka/pull/16345#discussion_r1643442351
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java:
##
@@ -785,6 +805,13 @@ void removeRequest() {
}
}
+// Visible for testing
+Optional lastEpochSentOnCommit() {
+return lastEpochSentOnCommit;
+}
+
+
+
Review Comment:
Sure, removed.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: consumer log fixes [kafka]
kirktrue commented on code in PR #16345:
URL: https://github.com/apache/kafka/pull/16345#discussion_r1643412421
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java:
##
@@ -785,6 +805,13 @@ void removeRequest() {
}
}
+// Visible for testing
+Optional lastEpochSentOnCommit() {
+return lastEpochSentOnCommit;
+}
+
+
+
Review Comment:
Super nit: extra whitespace not needed, right?
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/MembershipManagerImpl.java:
##
@@ -362,7 +362,7 @@ private void transitionTo(MemberState nextState) {
metricsManager.recordRebalanceStarted(time.milliseconds());
}
-log.trace("Member {} with epoch {} transitioned from {} to {}.",
memberId, memberEpoch, state, nextState);
+log.info("Member {} with epoch {} transitioned from {} to {}.",
memberId, memberEpoch, state, nextState);
Review Comment:
This one seems like it would make for a noisy log. What about `DEBUG` as a
compromise?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: consumer log fixes [kafka]
lianetm commented on PR #16345: URL: https://github.com/apache/kafka/pull/16345#issuecomment-2174253436 Thanks for the review @chia7712! All comments addressed. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR: consumer log fixes [kafka]
lianetm commented on code in PR #16345:
URL: https://github.com/apache/kafka/pull/16345#discussion_r1643334957
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java:
##
@@ -680,6 +694,7 @@ public NetworkClientDelegate.UnsentRequest
toUnsentRequest() {
}
if (memberInfo.memberEpoch.isPresent()) {
data =
data.setGenerationIdOrMemberEpoch(memberInfo.memberEpoch.get());
+lastEpochSentOnCommit =
Optional.of(memberInfo.memberEpoch.get());
Review Comment:
Both done, great catch the 2nd one! I added a test to make sure we got it
all 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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: consumer log fixes [kafka]
lianetm commented on code in PR #16345:
URL: https://github.com/apache/kafka/pull/16345#discussion_r1643334348
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java:
##
@@ -741,6 +756,9 @@ public void onResponse(final ClientResponse response) {
"failed with unknown member ID. " +
error.message()));
return;
} else if (error == Errors.STALE_MEMBER_EPOCH) {
+log.error("OffsetCommit failed for member {} with
stale member epoch error. Last epoch sent: {}",
+memberInfo.memberId.orElse("undefined"),
+lastEpochSentOnCommit.isPresent() ?
lastEpochSentOnCommit.get() : "None");
Review Comment:
Sure, 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.
To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR: consumer log fixes [kafka]
chia7712 commented on code in PR #16345:
URL: https://github.com/apache/kafka/pull/16345#discussion_r1641765619
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java:
##
@@ -680,6 +694,7 @@ public NetworkClientDelegate.UnsentRequest
toUnsentRequest() {
}
if (memberInfo.memberEpoch.isPresent()) {
data =
data.setGenerationIdOrMemberEpoch(memberInfo.memberEpoch.get());
+lastEpochSentOnCommit =
Optional.of(memberInfo.memberEpoch.get());
Review Comment:
It seems we can assign `memberInfo.memberEpoch` to `lastEpochSentOnCommit`
as `Optional` is a POJO
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java:
##
@@ -680,6 +694,7 @@ public NetworkClientDelegate.UnsentRequest
toUnsentRequest() {
}
if (memberInfo.memberEpoch.isPresent()) {
data =
data.setGenerationIdOrMemberEpoch(memberInfo.memberEpoch.get());
+lastEpochSentOnCommit =
Optional.of(memberInfo.memberEpoch.get());
Review Comment:
For another, should we reset `lastEpochSentOnCommit` to none if
`memberInfo.memberEpoch` is not defined?
##
clients/src/main/java/org/apache/kafka/clients/consumer/internals/CommitRequestManager.java:
##
@@ -741,6 +756,9 @@ public void onResponse(final ClientResponse response) {
"failed with unknown member ID. " +
error.message()));
return;
} else if (error == Errors.STALE_MEMBER_EPOCH) {
+log.error("OffsetCommit failed for member {} with
stale member epoch error. Last epoch sent: {}",
+memberInfo.memberId.orElse("undefined"),
+lastEpochSentOnCommit.isPresent() ?
lastEpochSentOnCommit.get() : "None");
Review Comment:
maybe we can replace "None" by "undefined" for consistency?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
