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 2021/08/30 08:19:39 UTC
[GitHub] [kafka] cadonna commented on a change in pull request #11228: KAFKA-12887 Skip some RuntimeExceptions from exception handler
cadonna commented on a change in pull request #11228:
URL: https://github.com/apache/kafka/pull/11228#discussion_r698280849
##########
File path: streams/src/main/java/org/apache/kafka/streams/KafkaStreams.java
##########
@@ -495,9 +498,23 @@ private void replaceStreamThread(final Throwable throwable) {
}
}
+ private StreamsUncaughtExceptionHandler.StreamThreadExceptionResponse getActionForThrowable(final Throwable throwable,
+ final StreamsUncaughtExceptionHandler streamsUncaughtExceptionHandler) {
Review comment:
```suggestion
private StreamsUncaughtExceptionHandler.StreamThreadExceptionResponse getActionForThrowable(final Throwable throwable,
final StreamsUncaughtExceptionHandler streamsUncaughtExceptionHandler) {
```
##########
File path: streams/src/main/java/org/apache/kafka/streams/KafkaStreams.java
##########
@@ -495,9 +498,23 @@ private void replaceStreamThread(final Throwable throwable) {
}
}
+ private StreamsUncaughtExceptionHandler.StreamThreadExceptionResponse getActionForThrowable(final Throwable throwable,
+ final StreamsUncaughtExceptionHandler streamsUncaughtExceptionHandler) {
+ final StreamsUncaughtExceptionHandler.StreamThreadExceptionResponse action;
+ // Exception might we wrapped within a StreamsException one
+ if (throwable.getCause() != null && EXCEPTIONS_NOT_TO_BE_HANDLED_BY_USERS.contains(throwable.getCause().getClass())) {
Review comment:
Could you please put this check in a method with a meaningful name like `wrappedExceptionIsIn()`? Then the check would be
```
if (wrappedExceptionIsIn(EXCEPTIONS_NOT_TO_BE_HANDLED_BY_USERS))
```
and you can remove all other inline comments because the code per se contains all needed info.
##########
File path: streams/src/test/java/org/apache/kafka/streams/integration/StreamsUncaughtExceptionHandlerIntegrationTest.java
##########
@@ -163,6 +164,27 @@ public void shouldShutdownClient() throws InterruptedException {
}
}
+
+ @Test
+ public void shouldShutdownClientWhenIllegalStateException() throws InterruptedException {
Review comment:
Could you please also add a test for the `IllegalArgumentException`?
--
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