You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "ilyazr (via GitHub)" <gi...@apache.org> on 2024/04/11 18:01:50 UTC
[PR] KAFKA-16466: add details of an internal exception to the failure message [kafka]
ilyazr opened a new pull request, #15701:
URL: https://github.com/apache/kafka/pull/15701
EventHandlerExceptionInfo#failureMessage() didn't include a message of the propagated exception. But it would be helpful to have them for non-fault exceptions. This PR adds it, so it can be helpful to debug errors
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16466: add details of an internal exception to the failure message [kafka]
Posted by "ilyazr (via GitHub)" <gi...@apache.org>.
ilyazr commented on PR #15701:
URL: https://github.com/apache/kafka/pull/15701#issuecomment-2068079457
@soarez
Hi! I've added some changes to fix those tests, but now it shows one failing check. Could you tell me what has gone wrong now?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16466: add details of an internal exception to the failure message [kafka]
Posted by "ilyazr (via GitHub)" <gi...@apache.org>.
ilyazr commented on code in PR #15701:
URL: https://github.com/apache/kafka/pull/15701#discussion_r1575051333
##########
metadata/src/test/java/org/apache/kafka/controller/errors/EventHandlerExceptionInfoTest.java:
##########
@@ -72,6 +73,11 @@ public class EventHandlerExceptionInfoTest {
new TimeoutException(),
() -> OptionalInt.of(1));
+ private static final EventHandlerExceptionInfo KAFKA_EXCEPTION =
+ EventHandlerExceptionInfo.fromInternal(
+ new KafkaException("Custom kafka exception message"),
+ () -> OptionalInt.of(1));
+
Review Comment:
Initially I thought it would be better to keep all `EventHandlerExceptionInfo` declarations at the same place. But as you mentioned, you would be even better to have it on a method level as it is used only once. Moved `TIMEOUT` to the method too
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16466: add details of an internal exception to the failure message [kafka]
Posted by "ilyazr (via GitHub)" <gi...@apache.org>.
ilyazr commented on PR #15701:
URL: https://github.com/apache/kafka/pull/15701#issuecomment-2050226155
@mumrah
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16466: add details of an internal exception to the failure message [kafka]
Posted by "soarez (via GitHub)" <gi...@apache.org>.
soarez commented on code in PR #15701:
URL: https://github.com/apache/kafka/pull/15701#discussion_r1574584174
##########
metadata/src/main/java/org/apache/kafka/controller/errors/EventHandlerExceptionInfo.java:
##########
@@ -191,6 +191,10 @@ public String failureMessage(
}
}
bld.append(".");
+ if (!isFault && internalException.getMessage() != null) {
+ bld.append(" Detailed exception message: ");
Review Comment:
The "Detailed " bit seems unecessary, maybe just "Exception message: "?
##########
metadata/src/test/java/org/apache/kafka/controller/errors/EventHandlerExceptionInfoTest.java:
##########
@@ -174,10 +180,18 @@ public void testUnexpectedBaseOffsetExceptionInfo() {
public void testUnexpectedBaseOffsetFailureMessage() {
assertEquals("event failed with UnexpectedBaseOffsetException (treated as " +
"NotControllerException) at epoch 123 in 90 microseconds. Renouncing leadership " +
- "and reverting to the last committed offset 456.",
+ "and reverting to the last committed offset 456. Detailed exception message: Wanted base offset 3, but the next offset was 4",
UNEXPECTED_END_OFFSET.failureMessage(123, OptionalLong.of(90L), true, 456L));
}
+ @Test
+ public void testKafkaExceptionFailureMessage() {
Review Comment:
Should we make it clear we're testing the "isFault" case here in the test name? Perhaps `testFaultExceptionFailureMessage`?
##########
metadata/src/test/java/org/apache/kafka/controller/errors/EventHandlerExceptionInfoTest.java:
##########
@@ -72,6 +73,11 @@ public class EventHandlerExceptionInfoTest {
new TimeoutException(),
() -> OptionalInt.of(1));
+ private static final EventHandlerExceptionInfo KAFKA_EXCEPTION =
+ EventHandlerExceptionInfo.fromInternal(
+ new KafkaException("Custom kafka exception message"),
+ () -> OptionalInt.of(1));
+
Review Comment:
It seems this is only used in `testKafkaExceptionFailureMessage`, can we move it there?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16466: add details of an internal exception to the failure message [kafka]
Posted by "ilyazr (via GitHub)" <gi...@apache.org>.
ilyazr commented on code in PR #15701:
URL: https://github.com/apache/kafka/pull/15701#discussion_r1575054055
##########
metadata/src/test/java/org/apache/kafka/controller/errors/EventHandlerExceptionInfoTest.java:
##########
@@ -174,10 +180,18 @@ public void testUnexpectedBaseOffsetExceptionInfo() {
public void testUnexpectedBaseOffsetFailureMessage() {
assertEquals("event failed with UnexpectedBaseOffsetException (treated as " +
"NotControllerException) at epoch 123 in 90 microseconds. Renouncing leadership " +
- "and reverting to the last committed offset 456.",
+ "and reverting to the last committed offset 456. Detailed exception message: Wanted base offset 3, but the next offset was 4",
UNEXPECTED_END_OFFSET.failureMessage(123, OptionalLong.of(90L), true, 456L));
}
+ @Test
+ public void testKafkaExceptionFailureMessage() {
Review Comment:
It definitely would be better to have a clear name for the test. I've changed it to the suggested one
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16466: add details of an internal exception to the failure message [kafka]
Posted by "ilyazr (via GitHub)" <gi...@apache.org>.
ilyazr commented on code in PR #15701:
URL: https://github.com/apache/kafka/pull/15701#discussion_r1575046814
##########
metadata/src/main/java/org/apache/kafka/controller/errors/EventHandlerExceptionInfo.java:
##########
@@ -191,6 +191,10 @@ public String failureMessage(
}
}
bld.append(".");
+ if (!isFault && internalException.getMessage() != null) {
+ bld.append(" Detailed exception message: ");
Review Comment:
Removed "Detailed" from the message
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16466: add details of an internal exception to the failure message [kafka]
Posted by "soarez (via GitHub)" <gi...@apache.org>.
soarez commented on PR #15701:
URL: https://github.com/apache/kafka/pull/15701#issuecomment-2071198673
Failing tests are unrelated, except for `org.apache.kafka.controller.QuorumControllerTest.testBootstrapZkMigrationRecord()` ([KAFKA-16602](https://issues.apache.org/jira/browse/KAFKA-16602)) which didn't reproduce locally.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16466: add details of an internal exception to the failure message [kafka]
Posted by "ilyazr (via GitHub)" <gi...@apache.org>.
ilyazr commented on PR #15701:
URL: https://github.com/apache/kafka/pull/15701#issuecomment-2070177669
@soarez
Hi! Thanks for pointing out problems with the tests. I didn't know that.
p.s: added suggested changes
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16466: add details of an internal exception to the failure message [kafka]
Posted by "soarez (via GitHub)" <gi...@apache.org>.
soarez merged PR #15701:
URL: https://github.com/apache/kafka/pull/15701
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org