KAFKA-9131: Remove dead code for handling timeout exception#7635
Conversation
|
@guozhangwang @hachikuji -- would appreciate a review of this PR (for details look into https://issues.apache.org/jira/browse/KAFKA-9131 -- seems we change some producer code with regard to handling exceptions? Would be good if you could verify that this PR makes sense). |
|
@vvcephei could you also take a look at this PR as well? |
|
@guozhangwang . Roger that. I'll take a look. |
vvcephei
left a comment
There was a problem hiding this comment.
Hey @gleb-kom ,
Thanks for the PR!
Can you go ahead and remove the @throws TimeoutException from the KafkaProducer#send javadoc as well?
Looking at the code in doSend it looks like it's no longer possible for that method to directly throw any ApiException (it would always pass it to the callback instead). I'll leave it to you if you also want to clean up the other @throws declarations in the javadoc that are also no longer true.
Other than the above, and the one inline comment I made, this looks good to me!
There was a problem hiding this comment.
Can you move the comments from L136-7 below the new block, so that it can maintain its association with the RetriableException? Maybe you can put it inside the right block to keep it from getting dissociated again.
There was a problem hiding this comment.
I moved the comment below as you asked. However, on a personal note I think the both blocks are the right ones as TimeoutException is a subclass of Retriable Exception.
There was a problem hiding this comment.
I see your point, but it seems like the comment is more acknowledging that using RetriableException as a catch-all for all "retriable exceptions" isn't guaranteed to work, although it's the best we can do right now. If I understood the consumer API, though, TimeoutException is recommended as the way to catch "timeout exceptions".
But this might also be me justifying the position after the fact... Anyway, thanks for making the change.
|
@guozhangwang @vvcephei |
There was a problem hiding this comment.
Remove this suppress and fix generic.
There was a problem hiding this comment.
specify generic -> MockProducer<byte[], byte[]>
There was a problem hiding this comment.
remove try-fail-catch and rewrite to
final StreamsException expected = assertThrows(StreamsException.class, () -> collector.flush());
assertTrue(expected.getCause() instanceof TimeoutException);
assertTrue(expected.getMessage().endsWith(topic1TimeoutHint));
There was a problem hiding this comment.
nit: avoid double blank } catch should we single blank } catch
There was a problem hiding this comment.
I am surprised the linter did not catch it before you.
|
Some nit about code style. Overall LGMT. Can merge after addressed. |
|
@mjsax |
|
Java 11 / 2.12 passed. Java 11 / 2.13 and Java8 failed (test results are not available any longer). Retest this please. \cc @vvcephei for merging. |
|
@gleb-kom I tried to merge the PR but found it needs some rebasing, could you do that? |
…ns in catch clause and move it to the callback
|
@guozhangwang done |
…ns in catch clause and move it to the callback
More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.
Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Committer Checklist (excluded from commit message)