KAFKA-16448: Fix processing exception handler#16663
Conversation
sebastienviale
changed the title
cadonna
left a comment
cadonna
left a comment
There was a problem hiding this comment.
Thanks for the PR, @sebastienviale, @loicgreffier, and @Dabz!
I have one nit!
| return true; | ||
| } | ||
|
|
||
| private void handleException(final Throwable e) { |
There was a problem hiding this comment.
nit:
| private void handleException(final Throwable e) { | |
| private void handleException(final Exception e) { |
There was a problem hiding this comment.
@sebastienviale found that failedProcessingException.getCause() returns a Throwable. I think casting would be overhead. Let's keep private void handleException(final Throwable e)
There was a problem hiding this comment.
Why did you change private void handleException(final Throwable e) to private void handleException(final Exception e)? The additional wrapping into an Exception is not worth changing the signature. Please revert to private void handleException(final Throwable e) and pass in failedProcessingException.getCause().
See my comment above.
There was a problem hiding this comment.
Reverted. Probably a mistake in a recent commit
| record = null; | ||
| throw new TaskCorruptedException(Collections.singleton(id)); | ||
| } | ||
| } catch (final FailedProcessingException failedProcessingException){ |
There was a problem hiding this comment.
Checkstyle is complaining here. You need to change it to the following:
| } catch (final FailedProcessingException failedProcessingException){ | |
| } catch (final FailedProcessingException failedProcessingException) { |
Co-authored-by: Dabz <d.gasparina@gmail.com> Co-authored-by: loicgreffier <loic.greffier@michelin.com>
loicgreffier
force-pushed
the
KAFKA-16448-Fix-processing-exception-handler
branch
from
a26bf2a to
11c2573
Compare
| } catch (final StreamsException exception) { | ||
| record = null; | ||
| throw exception; | ||
| } catch (final RuntimeException e) { |
There was a problem hiding this comment.
Didn't we agree on using Exception here instead of RuntimeException to be consistent with other handlers?
There was a problem hiding this comment.
yes indeed, I think I missed a commit.
Co-authored-by: Dabz <d.gasparina@gmail.com> Co-authored-by: loicgreffier <loic.greffier@michelin.com>
Co-authored-by: Dabz <d.gasparina@gmail.com> Co-authored-by: loicgreffier <loic.greffier@michelin.com>
|
Thanks for the follow-up PR. Merged to |
Co-authored-by: Dabz <d.gasparina@gmail.com> Co-authored-by: loicgreffier <loic.greffier@michelin.com> Minor code improvements across different classed, related to the `ProcessingExceptionHandler` implementation (KIP-1033). Reviewers: Bruno Cadonna <bruno@confluent.io>, Matthias J. Sax <matthias@confluent.io>
4 participants
This PR is part of KAFKA-16448 which aims to bring a ProcessingExceptionHandler to Kafka Streams in order to deal with exceptions that occur during processing.
This PR resolves some minor fixes
Jira: https://issues.apache.org/jira/browse/KAFKA-16448.
Contributors
@Dabz
@sebastienviale
@loicgreffier