-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
KAFKA-16331: Remove KafkaClientSupplier from unit tests #17330
Conversation
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.
Overall lgtm, but there are failing StreamsConfig
tests - not sure if they are related or not
@@ -1950,16 +1938,6 @@ public static class ProductionExceptionHandlerMock implements ProductionExceptio | |||
private TaskId expectedTaskId; | |||
private SerializationExceptionOrigin expectedSerializationExceptionOrigin; | |||
|
|||
// No args constructor, referred in StreamsConfigTest | |||
public ProductionExceptionHandlerMock() { |
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.
Why remove this? More for my own understanding
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.
Ups. That's actually a slip... need to keep this. Good catch.
We don't pass in a client-supplier into `StreamsProducer` any longer, so we can simplify `StreamsProducerTest` and remove client-supplier.
0c87c42
to
5129fc7
Compare
We don't pass in a client-supplier into `StreamsProducer` any longer, so we can simplify `StreamsProducerTest` and remove client-supplier. Reviewers: Bill Bejeck <[email protected]>
We don't pass in a client-supplier into
StreamsProducer
any longer, so we can simplify multiple tests by removing client-supplier.Should have done this in #17259 but missed it...