-
Notifications
You must be signed in to change notification settings - Fork 14.3k
KAFKA-19042: [1/N] Move ConsumerTopicCreationTest to client-integration-tests module #19283
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-19042: [1/N] Move ConsumerTopicCreationTest to client-integration-tests module #19283
Conversation
…ts module JIRA: KAFKA-19042 This patch moves ConsumerTopicCreationTest to the client-integration-tests and rewrite it as Java.
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.
Thanks for this PR, left a comment
Map<String, Object> consumerConfig = Map.of( | ||
CLIENT_ID_CONFIG, "ConsumerTestConsumer", | ||
GROUP_ID_CONFIG, "TestGroup", | ||
ALLOW_AUTO_CREATE_TOPICS_CONFIG, allowConsumerAutoCreateTopics, | ||
GROUP_PROTOCOL_CONFIG, protocol.name().toLowerCase(Locale.ROOT), | ||
BOOTSTRAP_SERVERS_CONFIG, bootstrapServer | ||
); | ||
return new KafkaConsumer<>(consumerConfig, new ByteArrayDeserializer(), new ByteArrayDeserializer()); |
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.
Should use cluster.consumer()
to create consumer.
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.
@frankvicky thanks for this patch. could you please refactor it to eliminate the duplicate code?
|
||
private Consumer<byte[], byte[]> createConsumer(ClusterInstance cluster, GroupProtocol protocol, boolean allowConsumerAutoCreateTopics) { | ||
Map<String, Object> consumerConfig = Map.of( | ||
CLIENT_ID_CONFIG, "ConsumerTestConsumer", |
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.
Is this necessary?
} | ||
|
||
private Set<String> getAllTopics(ClusterInstance cluster) { | ||
return cluster.brokers().values().iterator().next().metadataCache().getAllTopics(); |
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.
Could you please avoid accessing broker? Maybe we can use Admin
instead?
…s module (apache#19283) This patch moves `ConsumerTopicCreationTest` to the `client-integration-tests` and rewrite it as Java. The patch also streamlines the test flow. In the Scala version, there is a producer that produces messages, but this is not the main purpose of the `ConsumerTopicCreationTest`. Reviewers: Ken Huang <[email protected]>, Chia-Ping Tsai <[email protected]>
JIRA: KAFKA-19042
This patch moves
ConsumerTopicCreationTest
to theclient-integration-tests
and rewrite it as Java.The patch also streamlines the test flow.
In the Scala version, there is a producer that produces messages, but this is not the main purpose of the
ConsumerTopicCreationTest
.Reviewers: Ken Huang [email protected], Chia-Ping Tsai [email protected]