You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2022/10/31 17:14:34 UTC
[GitHub] [kafka] OmniaGM opened a new pull request, #12804: [Minior] [Test] KAFKA-14344 : Build EmbeddedKafkaCluster with common configs used for all clients
OmniaGM opened a new pull request, #12804:
URL: https://github.com/apache/kafka/pull/12804
jira [KAFKA-14344](https://issues.apache.org/jira/browse/KAFKA-14344)
This pr will simplify setting up MM2 integration test with AclAuthorizer enabled
*More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.*
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison merged pull request #12804: [Minior] [Test] KAFKA-14344 : Build EmbeddedKafkaCluster with common configs used for all clients
Posted by GitBox <gi...@apache.org>.
mimaison merged PR #12804:
URL: https://github.com/apache/kafka/pull/12804
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on a diff in pull request #12804: [Minior] [Test] KAFKA-14344 : Build EmbeddedKafkaCluster with common configs used for all clients
Posted by GitBox <gi...@apache.org>.
mimaison commented on code in PR #12804:
URL: https://github.com/apache/kafka/pull/12804#discussion_r1012744812
##########
connect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedKafkaCluster.java:
##########
@@ -106,12 +106,20 @@ public class EmbeddedKafkaCluster {
private final String[] currentBrokerLogDirs;
private final boolean hasListenerConfig;
+ final Map<String, String> additionalClientConfigs;
Review Comment:
What about using `clientConfigs`? If you agree let's rename the setters too.
##########
connect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedKafkaCluster.java:
##########
@@ -435,7 +445,9 @@ public Admin createAdminClient(Properties adminClientConfig) {
adminClientConfig.put(SslConfigs.SSL_TRUSTSTORE_PASSWORD_CONFIG, ((Password) brokerConfig.get(SslConfigs.SSL_TRUSTSTORE_PASSWORD_CONFIG)).value());
adminClientConfig.put(CommonClientConfigs.SECURITY_PROTOCOL_CONFIG, "SSL");
}
- return Admin.create(adminClientConfig);
+ Properties finalAdminConfig = Utils.mkProperties(additionalClientConfigs);
Review Comment:
In `createConsumer()` and `createProducer()` we add the custom client config first, here we do it the other way around. Let use similar logic if we can
##########
connect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedKafkaCluster.java:
##########
@@ -435,7 +445,9 @@ public Admin createAdminClient(Properties adminClientConfig) {
adminClientConfig.put(SslConfigs.SSL_TRUSTSTORE_PASSWORD_CONFIG, ((Password) brokerConfig.get(SslConfigs.SSL_TRUSTSTORE_PASSWORD_CONFIG)).value());
adminClientConfig.put(CommonClientConfigs.SECURITY_PROTOCOL_CONFIG, "SSL");
}
- return Admin.create(adminClientConfig);
+ Properties finalAdminConfig = Utils.mkProperties(additionalClientConfigs);
Review Comment:
I wonder if we could also change the type of `adminClientConfig` to `Map` so it's similar to the other create methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] OmniaGM commented on a diff in pull request #12804: [Minior] [Test] KAFKA-14344 : Build EmbeddedKafkaCluster with common configs used for all clients
Posted by GitBox <gi...@apache.org>.
OmniaGM commented on code in PR #12804:
URL: https://github.com/apache/kafka/pull/12804#discussion_r1012324985
##########
connect/mirror/src/test/java/org/apache/kafka/connect/mirror/integration/MirrorConnectorsIntegrationBaseTest.java:
##########
@@ -640,11 +645,7 @@ protected static void waitUntilMirrorMakerIsRunning(EmbeddedConnectCluster conne
* wait for the topic created on the cluster
*/
protected static void waitForTopicCreated(EmbeddedConnectCluster cluster, String topicName) throws InterruptedException {
- try (final Admin adminClient = cluster.kafka().createAdminClient()) {
- waitForCondition(() -> adminClient.listTopics().names().get().contains(topicName), TOPIC_SYNC_DURATION_MS,
- "Topic: " + topicName + " didn't get created on cluster: " + cluster.getName()
- );
- }
+ waitForTopicCreated(cluster.kafka(), topicName);
Review Comment:
yeah this will unify the code test utils more.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] OmniaGM commented on a diff in pull request #12804: [Minior] [Test] KAFKA-14344 : Build EmbeddedKafkaCluster with common configs used for all clients
Posted by GitBox <gi...@apache.org>.
OmniaGM commented on code in PR #12804:
URL: https://github.com/apache/kafka/pull/12804#discussion_r1014899756
##########
connect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedKafkaCluster.java:
##########
@@ -435,7 +445,9 @@ public Admin createAdminClient(Properties adminClientConfig) {
adminClientConfig.put(SslConfigs.SSL_TRUSTSTORE_PASSWORD_CONFIG, ((Password) brokerConfig.get(SslConfigs.SSL_TRUSTSTORE_PASSWORD_CONFIG)).value());
adminClientConfig.put(CommonClientConfigs.SECURITY_PROTOCOL_CONFIG, "SSL");
}
- return Admin.create(adminClientConfig);
+ Properties finalAdminConfig = Utils.mkProperties(additionalClientConfigs);
Review Comment:
unified the logic for `createConsumer`, `createProducer` and `adminClientConfig` including changing type of `adminClientConfig` to `Map`
##########
connect/runtime/src/test/java/org/apache/kafka/connect/util/clusters/EmbeddedKafkaCluster.java:
##########
@@ -106,12 +106,20 @@ public class EmbeddedKafkaCluster {
private final String[] currentBrokerLogDirs;
private final boolean hasListenerConfig;
+ final Map<String, String> additionalClientConfigs;
Review Comment:
done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on a diff in pull request #12804: [Minior] [Test] KAFKA-14344 : Build EmbeddedKafkaCluster with common configs used for all clients
Posted by GitBox <gi...@apache.org>.
mimaison commented on code in PR #12804:
URL: https://github.com/apache/kafka/pull/12804#discussion_r1012093538
##########
connect/mirror/src/test/java/org/apache/kafka/connect/mirror/integration/MirrorConnectorsIntegrationBaseTest.java:
##########
@@ -640,11 +645,7 @@ protected static void waitUntilMirrorMakerIsRunning(EmbeddedConnectCluster conne
* wait for the topic created on the cluster
*/
protected static void waitForTopicCreated(EmbeddedConnectCluster cluster, String topicName) throws InterruptedException {
- try (final Admin adminClient = cluster.kafka().createAdminClient()) {
- waitForCondition(() -> adminClient.listTopics().names().get().contains(topicName), TOPIC_SYNC_DURATION_MS,
- "Topic: " + topicName + " didn't get created on cluster: " + cluster.getName()
- );
- }
+ waitForTopicCreated(cluster.kafka(), topicName);
Review Comment:
Actually I wonder if we should delete the other `waitForTopicCreated()` method instead as it only has a single caller, and the caller can just use `backup` instead of `backup.kafka()`. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] OmniaGM commented on a diff in pull request #12804: [Minior] [Test] KAFKA-14344 : Build EmbeddedKafkaCluster with common configs used for all clients
Posted by GitBox <gi...@apache.org>.
OmniaGM commented on code in PR #12804:
URL: https://github.com/apache/kafka/pull/12804#discussion_r1012325212
##########
connect/mirror/src/test/java/org/apache/kafka/connect/mirror/integration/MirrorConnectorsIntegrationBaseTest.java:
##########
@@ -640,11 +645,7 @@ protected static void waitUntilMirrorMakerIsRunning(EmbeddedConnectCluster conne
* wait for the topic created on the cluster
*/
protected static void waitForTopicCreated(EmbeddedConnectCluster cluster, String topicName) throws InterruptedException {
- try (final Admin adminClient = cluster.kafka().createAdminClient()) {
- waitForCondition(() -> adminClient.listTopics().names().get().contains(topicName), TOPIC_SYNC_DURATION_MS,
- "Topic: " + topicName + " didn't get created on cluster: " + cluster.getName()
- );
- }
+ waitForTopicCreated(cluster.kafka(), topicName);
Review Comment:
updated the test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org