KAFKA-7551:Refactor to create producer & consumer in the worker. - #5842
KAFKA-7551:Refactor to create producer & consumer in the worker.#5842mageshn wants to merge 6 commits into
Conversation
|
retest this please |
1 similar comment
|
retest this please |
|
@ewencp Let me know if this looks good. |
rayokota
left a comment
rayokota
left a comment
There was a problem hiding this comment.
Just some minor nits, otherwise LGTM
rhauch
left a comment
rhauch
left a comment
There was a problem hiding this comment.
Thanks, @mageshn! The different locations where the producer and consumer are instantiated has bothered me for some time, so thanks for the taking the initiative to clean this up. I've left a few minor suggestions -- including adding a test or two to verify the assumptions that were in the original code, too.
|
retest this please |
|
@ewencp/ @hachikuji WDYT about this refactor? |
ewencp
left a comment
ewencp
left a comment
There was a problem hiding this comment.
Generally looks like a nice refactor and cleanup, just a few comments.
There was a problem hiding this comment.
connConfig is not used, we should just remove the parameter
There was a problem hiding this comment.
I can't remember all the details of when the powermock runner ends up being required, but this looks like we may have removed any need for powermock in this test other than Whitebox which may not need it. Would be worth figuring out if we can clean out some powermock usage since it's mostly only used in Connect and has caused problems with newer JDKs.
We don't need to do this here, but if we clean up uses of PowerMock we should consider doing cleanup passes for things like the test runner. Alternatively, might make sense to, at some point, do a JIRA dedicated to just removing use of powermock.
There was a problem hiding this comment.
I have created a test to address this for Connect. I haven't looked upto see if other components are using it as well. If they do, I will create tickets for those as well. https://issues.apache.org/jira/browse/KAFKA-7686
There was a problem hiding this comment.
I'm fine with these as is, but you could also change the methods to be static in Worker and accept the WorkerConfig as a parameter since that's the only class member they use. Would get rid of all the distracting mocks and expect calls and focus the tests on the key functionality of those methods.
There was a problem hiding this comment.
indentation issue here, if this is passing checkstyle i would be surprised
There was a problem hiding this comment.
This is no longer applicable but AFAIK, this did pass checkstyle.
Worker Task refactoring mark the new methods private
|
@ewencp This is ready for another pass. Can you please take another look at it. |
ewencp
left a comment
ewencp
left a comment
There was a problem hiding this comment.
LGTM, thanks for the clean up!
This is minor refactoring that brings in the creation of producer and consumer to the Worker. Currently, the consumer is created in the WorkerSinkTask. This should not affect any functionality and it just makes the code structure easier to understand. Author: Magesh Nandakumar <magesh.n.kumar@gmail.com> Reviewers: Ryanne Dolan <ryannedolan@gmail.com>, Randall Hauch <rhauch@gmail.com>, Robert Yokota <rayokota@gmail.com>, Ewen Cheslack-Postava <ewen@confluent.io> Closes apache#5842 from mageshn/KAFKA-7551
6 participants
This is minor refactoring that brings in the creation of producer and consumer to the Worker. Currently, the consumer is created in the WorkerSinkTask. This should not affect any functionality and it just makes the code structure easier to understand.
Committer Checklist (excluded from commit message)