KAFKA-14007: Invoking connect headers.close method on shutdown#12309
KAFKA-14007: Invoking connect headers.close method on shutdown#12309C0urante merged 5 commits intoapache:trunkfrom
Conversation
|
@C0urante , plz review whenever you get the chance. |
|
@vamossagar12 there are test failures because the calls to For future reference, you can run unit tests locally with |
|
@C0urante , I did run the unit tests before pushing the changes the first time around but instead of running connect:unitTests, ran it for some other sub-task. Sorry about that |
There was a problem hiding this comment.
Looks good! Two small comments but apart from that, 👍
BTW, regarding this:
I did run the unit tests before pushing the changes the first time around but instead of running connect:unitTests, ran it for some other sub-task.
I think that's the problem--funnily enough, the :connect:unitTest task (and also :connect:test) doesn't actually run any tests. You want to use connect:runtime:unitTest (or connect:runtime:test) instead.
| this.serializer.close(); | ||
| this.deserializer.close(); |
There was a problem hiding this comment.
Good catch here. Should we do the same in the NumberConverter class?
There was a problem hiding this comment.
Also, even though these are no-ops right now, if we want to be meticulous about the cleanup logic here, should we wrap both of these in Utils::closeQuietly, so that if one fails, we still get a chance to close the other?
Thanks. I have made those changes. Also, thanks for the information on the connect test tasks 👍 |
connect/json/src/main/java/org/apache/kafka/connect/json/JsonConverter.java
Outdated
Show resolved
Hide resolved
…onverter.java Co-authored-by: Chris Egerton <fearthecellos@gmail.com>
|
|
||
| @Override | ||
| public void close() { | ||
| Utils.closeQuietly(this.serializer, "number format serializer"); |
There was a problem hiding this comment.
What is format? Would number converter serializer or even just number serializer make more sense here?
There was a problem hiding this comment.
I changed it to number converter serializer. Somehow had NumberFormatException in my mind when I was making this change and ended up writing number format serializer
C0urante
left a comment
C0urante
left a comment
There was a problem hiding this comment.
LGTM, one more small comment. You'll also want to update the JsonConverter to import the Utils class or the class won't compile.
C0urante
left a comment
C0urante
left a comment
There was a problem hiding this comment.
LGTM, thanks @vamossagar12
|
Thanks @C0urante . How to get this PR merged though :) |
|
This needs to be reviewed by a committer now. @showuon might be able to take a look? |
|
@showuon , can you plz review the changes? Thanks! |
|
@showuon could you plz review/merge this? Thanks! |
|
@C0urante same as the other PR, I think you can merge this now :) |
|
@vamossagar12 ah yeah, thanks for the ping! I'll merge this one tonight and take a final look at #12321 tomorrow; please ping me again if there's no activity on it by the end of the day. |
|
Thanks @C0urante ! |
3 participants
The HeaderConverter interface extends Closeable, but HeaderConverter::close is never actually invoked anywhere. We can and should start invoking it, probably wrapped in Utils::closeQuietly so that any invalid logic in that method for custom header converters that has to date gone undetected will not cause new task failures.
This PR addresses the above concern by invoking HeaderConverter.close in cases of shutdowns.