-
Notifications
You must be signed in to change notification settings - Fork 14.4k
KAFKA-19110: Add missing unit test for Streams-consumer integration #19457
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: PoAn Yang <payang@apache.org>
Signed-off-by: PoAn Yang <payang@apache.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looking good to me, just one minor thing. Could you fix this as part of this PR? Then I think it is ready to merge from my side.
HANDLER | ||
) | ||
); | ||
assertEquals("Named topologies and the CONSUMER protocol cannot be used at the same time.", exception.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exception message is actually wrong. "Named topologies and the STREAMS protocol" should be what it says. Can you update the test and the production code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed it. Thanks.
Signed-off-by: PoAn Yang <payang@apache.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a couple of optional comments to consider
@@ -220,7 +228,7 @@ private AsyncKafkaConsumer<String, String> newConsumer(Properties props) { | |||
(a, b, c, d, e, f, g) -> fetchCollector, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is pre-existing, but I'm wondering if it's worth doing a follow-up PR to use more meaningful names than a
, b
etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will handle this in a follow-up PR. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Follow-up PR: #19550
taskManager | ||
); | ||
|
||
private void setup(final StreamsRebalanceData streamsRebalanceData) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: It took a second to realize what was going on with creating a DefaultStreamsRebalanceListener
in two places. Maybe change the name from setup()
with something like createRebalanceListenerWithRebalanceData
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it. Thanks.
Signed-off-by: PoAn Yang <payang@apache.org>
Replace names like a, b, c, ... with meaningful names in AsyncKafkaConsumerTest. Follow-up: #19457 (comment) Signed-off-by: PoAn Yang <payang@apache.org> Reviewers: Bill Bejeck <bbejeck@apache.org>, Ken Huang <s7133700@gmail.com>
AsyncKafkaConsumer
constructor and verify that theRequestManagers.supplier()
contains Streams-specific data structures.RequestManagers
constructs the Streams request managerscorrectly
StreamsGroupHeartbeatManager#resetPollTimer()
StreamsOnTasksRevokedCallbackCompletedEvent
,StreamsOnTasksAssignedCallbackCompletedEvent
, andStreamsOnAllTasksLostCallbackCompletedEvent
inApplicationEventProcessor
DefaultStreamsRebalanceListener
StreamThread
.handleStreamsRebalanceData
.StreamsRebalanceData
.Reviewers: Lucas Brutschy lbrutschy@confluent.io