Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
TaiJuWu commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1676524880
##
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##
@@ -706,4 +706,9 @@ public static ApiVersionsResponse createApiVersionsResponse(
ApiVersionsResponse.UNKNOWN_FINALIZED_FEATURES_EPOCH,
zkMigrationEnabled);
}
+
+public static String getCurrentFunctionName() {
Review Comment:
Thanks for your info. Update already.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
chia7712 commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1676512274
##
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##
@@ -706,4 +706,9 @@ public static ApiVersionsResponse createApiVersionsResponse(
ApiVersionsResponse.UNKNOWN_FINALIZED_FEATURES_EPOCH,
zkMigrationEnabled);
}
+
+public static String getCurrentFunctionName() {
Review Comment:
> Yes, they are same problem.
Have you test it on local? `testInfo.getTestMethod().get().getName()` show
the "pure" method name on my local.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
TaiJuWu commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1675473241
##
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##
@@ -706,4 +706,9 @@ public static ApiVersionsResponse createApiVersionsResponse(
ApiVersionsResponse.UNKNOWN_FINALIZED_FEATURES_EPOCH,
zkMigrationEnabled);
}
+
+public static String getCurrentFunctionName() {
Review Comment:
> Have you toke a look at `testInfo.getTestMethod()`?
Yes, they are same problem.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
TaiJuWu commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1675473241
##
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##
@@ -706,4 +706,9 @@ public static ApiVersionsResponse createApiVersionsResponse(
ApiVersionsResponse.UNKNOWN_FINALIZED_FEATURES_EPOCH,
zkMigrationEnabled);
}
+
+public static String getCurrentFunctionName() {
Review Comment:
> Have you toke a look at `testInfo.getTestMethod()`?
Yes, they are some problem.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
chia7712 commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1675282098
##
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##
@@ -706,4 +706,9 @@ public static ApiVersionsResponse createApiVersionsResponse(
ApiVersionsResponse.UNKNOWN_FINALIZED_FEATURES_EPOCH,
zkMigrationEnabled);
}
+
+public static String getCurrentFunctionName() {
Review Comment:
Have you toke a look at `testInfo.getTestMethod()`?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
TaiJuWu commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1673847880
##
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##
@@ -706,4 +706,9 @@ public static ApiVersionsResponse createApiVersionsResponse(
ApiVersionsResponse.UNKNOWN_FINALIZED_FEATURES_EPOCH,
zkMigrationEnabled);
}
+
+public static String getCurrentFunctionName() {
Review Comment:
Unfortunately ,we can't use `testInfo`, use `testCreateWithDefaults` as
example, the name from `testInfo.getDisplayName()` will get
`testCreateWithDefaults [1] Type=Raft-Isolated,
MetadataVersion=4.0-IV0,Security=PLAINTEXT` and it is illegal name.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
TaiJuWu commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1673847880
##
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##
@@ -706,4 +706,9 @@ public static ApiVersionsResponse createApiVersionsResponse(
ApiVersionsResponse.UNKNOWN_FINALIZED_FEATURES_EPOCH,
zkMigrationEnabled);
}
+
+public static String getCurrentFunctionName() {
Review Comment:
We can't use `testInfo`, use `testCreateWithDefaults` as example, the name
from `testInfo.getDisplayName()` will get `testCreateWithDefaults [1]
Type=Raft-Isolated, MetadataVersion=4.0-IV0,Security=PLAINTEXT` and it is
illegal name.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
TaiJuWu commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1673848590
##
core/src/test/java/kafka/testkit/KafkaClusterTestKit.java:
##
@@ -191,7 +191,7 @@ private KafkaConfig createNodeConfig(TestKitNode node) {
controllerNode.metadataDirectory());
}
props.put(SocketServerConfigs.LISTENER_SECURITY_PROTOCOL_MAP_CONFIG,
-"EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT");
+
"EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT,PLAINTEXT:PLAINTEXT");
Review Comment:
revert, thanks for your remind.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
TaiJuWu commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1673847880
##
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##
@@ -706,4 +706,9 @@ public static ApiVersionsResponse createApiVersionsResponse(
ApiVersionsResponse.UNKNOWN_FINALIZED_FEATURES_EPOCH,
zkMigrationEnabled);
}
+
+public static String getCurrentFunctionName() {
Review Comment:
We can't use `testInfo`, using `testCreateWithDefaults` as example, the name
from `testInfo.getDisplayName()` will get `testCreateWithDefaults [1]
Type=Raft-Isolated, MetadataVersion=4.0-IV0,Security=PLAINTEXT` and it is
illegal name.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
TaiJuWu commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1673195870
##
core/src/test/java/kafka/testkit/KafkaClusterTestKit.java:
##
@@ -191,7 +191,7 @@ private KafkaConfig createNodeConfig(TestKitNode node) {
controllerNode.metadataDirectory());
}
props.put(SocketServerConfigs.LISTENER_SECURITY_PROTOCOL_MAP_CONFIG,
-"EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT");
+
"EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT,PLAINTEXT:PLAINTEXT");
Review Comment:
I will check again and try to remove .
Thanks for your review.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
TaiJuWu commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1673195870
##
core/src/test/java/kafka/testkit/KafkaClusterTestKit.java:
##
@@ -191,7 +191,7 @@ private KafkaConfig createNodeConfig(TestKitNode node) {
controllerNode.metadataDirectory());
}
props.put(SocketServerConfigs.LISTENER_SECURITY_PROTOCOL_MAP_CONFIG,
-"EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT");
+
"EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT,PLAINTEXT:PLAINTEXT");
Review Comment:
I will check again.
Thanks for your review.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
TaiJuWu commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1673194862
##
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##
@@ -706,4 +706,9 @@ public static ApiVersionsResponse createApiVersionsResponse(
ApiVersionsResponse.UNKNOWN_FINALIZED_FEATURES_EPOCH,
zkMigrationEnabled);
}
+
+public static String getCurrentFunctionName() {
Review Comment:
Sorry, I missed the newset commend.
Will do ASAP.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
chia7712 commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1673151480
##
core/src/test/java/kafka/testkit/KafkaClusterTestKit.java:
##
@@ -191,7 +191,7 @@ private KafkaConfig createNodeConfig(TestKitNode node) {
controllerNode.metadataDirectory());
}
props.put(SocketServerConfigs.LISTENER_SECURITY_PROTOCOL_MAP_CONFIG,
-"EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT");
+
"EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT,PLAINTEXT:PLAINTEXT");
Review Comment:
Do we still need those changes?
##
clients/src/test/java/org/apache/kafka/test/TestUtils.java:
##
@@ -706,4 +706,9 @@ public static ApiVersionsResponse createApiVersionsResponse(
ApiVersionsResponse.UNKNOWN_FINALIZED_FEATURES_EPOCH,
zkMigrationEnabled);
}
+
+public static String getCurrentFunctionName() {
Review Comment:
IIRC, we had a discussion about the topic name. What is the updates?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
chia7712 commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1667735984
##
tools/src/test/java/org/apache/kafka/tools/TopicCommandIntegrationTest.java:
##
@@ -86,652 +85,952 @@
@Tag("integration")
@SuppressWarnings("deprecation") // Added for Scala 2.12 compatibility for
usages of JavaConverters
-public class TopicCommandIntegrationTest extends
kafka.integration.KafkaServerTestHarness implements Logging, RackAwareTest {
+@ExtendWith(ClusterTestExtensions.class)
+public class TopicCommandIntegrationTest {
private final short defaultReplicationFactor = 1;
private final int defaultNumPartitions = 1;
-private TopicCommand.TopicService topicService;
-private Admin adminClient;
-private String bootstrapServer;
-private String testTopicName;
-private Buffer scalaBrokers;
-private Seq scalaControllers;
-/**
- * Implementations must override this method to return a set of
KafkaConfigs. This method will be invoked for every
- * test and should not reuse previous configurations unless they select
their ports randomly when servers are started.
- *
- * Note the replica fetch max bytes is set to `1` in order to throttle the
rate of replication for test
- * `testDescribeUnderReplicatedPartitionsWhenReassignmentIsInProgress`.
- */
-@Override
-public scala.collection.Seq generateConfigs() {
-Map rackInfo = new HashMap<>();
-rackInfo.put(0, "rack1");
-rackInfo.put(1, "rack2");
-rackInfo.put(2, "rack2");
-rackInfo.put(3, "rack1");
-rackInfo.put(4, "rack3");
-rackInfo.put(5, "rack3");
-
-List brokerConfigs = ToolsTestUtils
-.createBrokerProperties(6, zkConnectOrNull(), rackInfo,
defaultNumPartitions, defaultReplicationFactor);
-
-List configs = new ArrayList<>();
-for (Properties props : brokerConfigs) {
-props.put(REPLICA_FETCH_MAX_BYTES_CONFIG, "1");
-configs.add(KafkaConfig.fromProps(props));
-}
-return JavaConverters.asScalaBuffer(configs).toSeq();
-}
+private final ClusterInstance clusterInstance;
private TopicCommand.TopicCommandOptions
buildTopicCommandOptionsWithBootstrap(String... opts) {
+String bootstrapServer = clusterInstance.bootstrapServers();
String[] finalOptions = Stream.concat(Arrays.stream(opts),
Stream.of("--bootstrap-server", bootstrapServer)
).toArray(String[]::new);
return new TopicCommand.TopicCommandOptions(finalOptions);
}
-@BeforeEach
-public void setUp(TestInfo info) {
-super.setUp(info);
-// create adminClient
-Properties props = new Properties();
-bootstrapServer = bootstrapServers(listenerName());
-props.put(CommonClientConfigs.BOOTSTRAP_SERVERS_CONFIG,
bootstrapServer);
-adminClient = Admin.create(props);
-topicService = new TopicCommand.TopicService(props,
Optional.of(bootstrapServer));
-testTopicName = String.format("%s-%s",
info.getTestMethod().get().getName(),
org.apache.kafka.test.TestUtils.randomString(10));
-scalaBrokers = brokers();
-scalaControllers = controllerServers();
+static List generate1() {
+Map serverProp = new HashMap<>();
+serverProp.put(REPLICA_FETCH_MAX_BYTES_CONFIG, "1"); // if config name
error, no exception throw
+
+Map> rackInfo = new HashMap<>();
+Map infoPerBroker1 = new HashMap<>();
+infoPerBroker1.put("broker.rack", "rack1");
+Map infoPerBroker2 = new HashMap<>();
+infoPerBroker2.put("broker.rack", "rack2");
+Map infoPerBroker3 = new HashMap<>();
+infoPerBroker3.put("broker.rack", "rack2");
+Map infoPerBroker4 = new HashMap<>();
+infoPerBroker4.put("broker.rack", "rack1");
+Map infoPerBroker5 = new HashMap<>();
+infoPerBroker5.put("broker.rack", "rack3");
+Map infoPerBroker6 = new HashMap<>();
+infoPerBroker6.put("broker.rack", "rack3");
+
+rackInfo.put(0, infoPerBroker1);
+rackInfo.put(1, infoPerBroker2);
+rackInfo.put(2, infoPerBroker3);
+rackInfo.put(3, infoPerBroker4);
+rackInfo.put(4, infoPerBroker5);
+rackInfo.put(5, infoPerBroker6);
+
+return Collections.singletonList(ClusterConfig.defaultBuilder()
+.setBrokers(6)
+.setServerProperties(serverProp)
+.setPerServerProperties(rackInfo)
+.build()
+);
}
-@AfterEach
-public void close() throws Exception {
-if (topicService != null)
-topicService.close();
-if (adminClient != null)
-adminClient.close();
+TopicCommandIntegrationTest(ClusterInstance clusterInstance) {
+this.clusterInstance = clusterInstance;
}
-@ParameterizedTest
-@ValueSource(strings =
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
TaiJuWu commented on PR #16127: URL: https://github.com/apache/kafka/pull/16127#issuecomment-2211590968 There are some tests fail, I am going to dig it. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
TaiJuWu commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1667232604
##
tools/src/test/java/org/apache/kafka/tools/TopicCommandIntegrationTest.java:
##
@@ -86,652 +85,952 @@
@Tag("integration")
@SuppressWarnings("deprecation") // Added for Scala 2.12 compatibility for
usages of JavaConverters
-public class TopicCommandIntegrationTest extends
kafka.integration.KafkaServerTestHarness implements Logging, RackAwareTest {
+@ExtendWith(ClusterTestExtensions.class)
+public class TopicCommandIntegrationTest {
private final short defaultReplicationFactor = 1;
private final int defaultNumPartitions = 1;
-private TopicCommand.TopicService topicService;
-private Admin adminClient;
-private String bootstrapServer;
-private String testTopicName;
-private Buffer scalaBrokers;
-private Seq scalaControllers;
-/**
- * Implementations must override this method to return a set of
KafkaConfigs. This method will be invoked for every
- * test and should not reuse previous configurations unless they select
their ports randomly when servers are started.
- *
- * Note the replica fetch max bytes is set to `1` in order to throttle the
rate of replication for test
- * `testDescribeUnderReplicatedPartitionsWhenReassignmentIsInProgress`.
- */
-@Override
-public scala.collection.Seq generateConfigs() {
-Map rackInfo = new HashMap<>();
-rackInfo.put(0, "rack1");
-rackInfo.put(1, "rack2");
-rackInfo.put(2, "rack2");
-rackInfo.put(3, "rack1");
-rackInfo.put(4, "rack3");
-rackInfo.put(5, "rack3");
-
-List brokerConfigs = ToolsTestUtils
-.createBrokerProperties(6, zkConnectOrNull(), rackInfo,
defaultNumPartitions, defaultReplicationFactor);
-
-List configs = new ArrayList<>();
-for (Properties props : brokerConfigs) {
-props.put(REPLICA_FETCH_MAX_BYTES_CONFIG, "1");
-configs.add(KafkaConfig.fromProps(props));
-}
-return JavaConverters.asScalaBuffer(configs).toSeq();
-}
+private final ClusterInstance clusterInstance;
private TopicCommand.TopicCommandOptions
buildTopicCommandOptionsWithBootstrap(String... opts) {
+String bootstrapServer = clusterInstance.bootstrapServers();
String[] finalOptions = Stream.concat(Arrays.stream(opts),
Stream.of("--bootstrap-server", bootstrapServer)
).toArray(String[]::new);
return new TopicCommand.TopicCommandOptions(finalOptions);
}
-@BeforeEach
-public void setUp(TestInfo info) {
-super.setUp(info);
-// create adminClient
-Properties props = new Properties();
-bootstrapServer = bootstrapServers(listenerName());
-props.put(CommonClientConfigs.BOOTSTRAP_SERVERS_CONFIG,
bootstrapServer);
-adminClient = Admin.create(props);
-topicService = new TopicCommand.TopicService(props,
Optional.of(bootstrapServer));
-testTopicName = String.format("%s-%s",
info.getTestMethod().get().getName(),
org.apache.kafka.test.TestUtils.randomString(10));
-scalaBrokers = brokers();
-scalaControllers = controllerServers();
+static List generate1() {
+Map serverProp = new HashMap<>();
+serverProp.put(REPLICA_FETCH_MAX_BYTES_CONFIG, "1"); // if config name
error, no exception throw
+
+Map> rackInfo = new HashMap<>();
+Map infoPerBroker1 = new HashMap<>();
+infoPerBroker1.put("broker.rack", "rack1");
+Map infoPerBroker2 = new HashMap<>();
+infoPerBroker2.put("broker.rack", "rack2");
+Map infoPerBroker3 = new HashMap<>();
+infoPerBroker3.put("broker.rack", "rack2");
+Map infoPerBroker4 = new HashMap<>();
+infoPerBroker4.put("broker.rack", "rack1");
+Map infoPerBroker5 = new HashMap<>();
+infoPerBroker5.put("broker.rack", "rack3");
+Map infoPerBroker6 = new HashMap<>();
+infoPerBroker6.put("broker.rack", "rack3");
+
+rackInfo.put(0, infoPerBroker1);
+rackInfo.put(1, infoPerBroker2);
+rackInfo.put(2, infoPerBroker3);
+rackInfo.put(3, infoPerBroker4);
+rackInfo.put(4, infoPerBroker5);
+rackInfo.put(5, infoPerBroker6);
+
+return Collections.singletonList(ClusterConfig.defaultBuilder()
+.setBrokers(6)
+.setServerProperties(serverProp)
+.setPerServerProperties(rackInfo)
+.build()
+);
}
-@AfterEach
-public void close() throws Exception {
-if (topicService != null)
-topicService.close();
-if (adminClient != null)
-adminClient.close();
+TopicCommandIntegrationTest(ClusterInstance clusterInstance) {
+this.clusterInstance = clusterInstance;
}
-@ParameterizedTest
-@ValueSource(strings =
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
TaiJuWu commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1667216599
##
core/src/test/java/kafka/test/junit/RaftClusterInvocationContext.java:
##
@@ -189,7 +189,9 @@ public void start() {
if (started.compareAndSet(false, true)) {
clusterTestKit.startup();
kafka.utils.TestUtils.waitUntilTrue(
-() ->
this.clusterTestKit.brokers().get(0).brokerState() == BrokerState.RUNNING,
Review Comment:
I open a new PR for this change.
Please take a look [here](https://github.com/apache/kafka/pull/16537)
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
TaiJuWu commented on code in PR #16127: URL: https://github.com/apache/kafka/pull/16127#discussion_r1667203653 ## core/src/test/java/kafka/testkit/KafkaClusterTestKit.java: ## @@ -356,18 +357,19 @@ private static void setupNodeDirectories(File baseDirectory, private final TestKitNodes nodes; private final Map controllers; private final Map brokers; + private final ControllerQuorumVotersFutureManager controllerQuorumVotersFutureManager; private final File baseDirectory; private final SimpleFaultHandlerFactory faultHandlerFactory; private KafkaClusterTestKit( -ExecutorService executorService, -TestKitNodes nodes, -Map controllers, -Map brokers, -ControllerQuorumVotersFutureManager controllerQuorumVotersFutureManager, -File baseDirectory, -SimpleFaultHandlerFactory faultHandlerFactory +ExecutorService executorService, Review Comment: Ok. Please wait a moment. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
chia7712 commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r135563
##
core/src/test/java/kafka/testkit/KafkaClusterTestKit.java:
##
@@ -390,7 +392,7 @@ public void format() throws Exception {
BrokerServer broker = entry.getValue();
futures.add(executorService.submit(() -> {
formatNode(broker.sharedServer().metaPropsEnsemble(),
-!nodes().brokerNodes().get(entry.getKey()).combined());
+
!nodes().brokerNodes().get(entry.getKey()).combined());
Review Comment:
ditto
##
tools/src/test/java/org/apache/kafka/tools/TopicCommandIntegrationTest.java:
##
@@ -86,652 +85,952 @@
@Tag("integration")
@SuppressWarnings("deprecation") // Added for Scala 2.12 compatibility for
usages of JavaConverters
-public class TopicCommandIntegrationTest extends
kafka.integration.KafkaServerTestHarness implements Logging, RackAwareTest {
+@ExtendWith(ClusterTestExtensions.class)
+public class TopicCommandIntegrationTest {
private final short defaultReplicationFactor = 1;
private final int defaultNumPartitions = 1;
-private TopicCommand.TopicService topicService;
-private Admin adminClient;
-private String bootstrapServer;
-private String testTopicName;
-private Buffer scalaBrokers;
-private Seq scalaControllers;
-/**
- * Implementations must override this method to return a set of
KafkaConfigs. This method will be invoked for every
- * test and should not reuse previous configurations unless they select
their ports randomly when servers are started.
- *
- * Note the replica fetch max bytes is set to `1` in order to throttle the
rate of replication for test
- * `testDescribeUnderReplicatedPartitionsWhenReassignmentIsInProgress`.
- */
-@Override
-public scala.collection.Seq generateConfigs() {
-Map rackInfo = new HashMap<>();
-rackInfo.put(0, "rack1");
-rackInfo.put(1, "rack2");
-rackInfo.put(2, "rack2");
-rackInfo.put(3, "rack1");
-rackInfo.put(4, "rack3");
-rackInfo.put(5, "rack3");
-
-List brokerConfigs = ToolsTestUtils
-.createBrokerProperties(6, zkConnectOrNull(), rackInfo,
defaultNumPartitions, defaultReplicationFactor);
-
-List configs = new ArrayList<>();
-for (Properties props : brokerConfigs) {
-props.put(REPLICA_FETCH_MAX_BYTES_CONFIG, "1");
-configs.add(KafkaConfig.fromProps(props));
-}
-return JavaConverters.asScalaBuffer(configs).toSeq();
-}
+private final ClusterInstance clusterInstance;
private TopicCommand.TopicCommandOptions
buildTopicCommandOptionsWithBootstrap(String... opts) {
+String bootstrapServer = clusterInstance.bootstrapServers();
String[] finalOptions = Stream.concat(Arrays.stream(opts),
Stream.of("--bootstrap-server", bootstrapServer)
).toArray(String[]::new);
return new TopicCommand.TopicCommandOptions(finalOptions);
}
-@BeforeEach
-public void setUp(TestInfo info) {
-super.setUp(info);
-// create adminClient
-Properties props = new Properties();
-bootstrapServer = bootstrapServers(listenerName());
-props.put(CommonClientConfigs.BOOTSTRAP_SERVERS_CONFIG,
bootstrapServer);
-adminClient = Admin.create(props);
-topicService = new TopicCommand.TopicService(props,
Optional.of(bootstrapServer));
-testTopicName = String.format("%s-%s",
info.getTestMethod().get().getName(),
org.apache.kafka.test.TestUtils.randomString(10));
-scalaBrokers = brokers();
-scalaControllers = controllerServers();
+static List generate1() {
+Map serverProp = new HashMap<>();
+serverProp.put(REPLICA_FETCH_MAX_BYTES_CONFIG, "1"); // if config name
error, no exception throw
+
+Map> rackInfo = new HashMap<>();
+Map infoPerBroker1 = new HashMap<>();
+infoPerBroker1.put("broker.rack", "rack1");
+Map infoPerBroker2 = new HashMap<>();
+infoPerBroker2.put("broker.rack", "rack2");
+Map infoPerBroker3 = new HashMap<>();
+infoPerBroker3.put("broker.rack", "rack2");
+Map infoPerBroker4 = new HashMap<>();
+infoPerBroker4.put("broker.rack", "rack1");
+Map infoPerBroker5 = new HashMap<>();
+infoPerBroker5.put("broker.rack", "rack3");
+Map infoPerBroker6 = new HashMap<>();
+infoPerBroker6.put("broker.rack", "rack3");
+
+rackInfo.put(0, infoPerBroker1);
+rackInfo.put(1, infoPerBroker2);
+rackInfo.put(2, infoPerBroker3);
+rackInfo.put(3, infoPerBroker4);
+rackInfo.put(4, infoPerBroker5);
+rackInfo.put(5, infoPerBroker6);
+
+return Collections.singletonList(ClusterConfig.defaultBuilder()
+
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
chia7712 commented on PR #16127: URL: https://github.com/apache/kafka/pull/16127#issuecomment-2194631598 @TaiJuWu please take a look at unrelated changes. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
chia7712 commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1632376721
##
core/src/test/java/kafka/testkit/KafkaClusterTestKit.java:
##
@@ -188,12 +188,13 @@ private KafkaConfig createNodeConfig(TestKitNode node) {
controllerNode.metadataDirectory());
}
props.put(SocketServerConfigs.LISTENER_SECURITY_PROTOCOL_MAP_CONFIG,
-"EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT");
+
"EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT,PLAINTEXT:PLAINTEXT");
Review Comment:
that is good. If you feel this PR needs some helpers, we can have another PR
to address that.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
TaiJuWu commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1632369365
##
core/src/test/java/kafka/testkit/KafkaClusterTestKit.java:
##
@@ -188,12 +188,13 @@ private KafkaConfig createNodeConfig(TestKitNode node) {
controllerNode.metadataDirectory());
}
props.put(SocketServerConfigs.LISTENER_SECURITY_PROTOCOL_MAP_CONFIG,
-"EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT");
+
"EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT,PLAINTEXT:PLAINTEXT");
Review Comment:
In this PR, I try to minimal this patch so I don't touch any helper function.
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
chia7712 commented on code in PR #16127: URL: https://github.com/apache/kafka/pull/16127#discussion_r1632358372 ## core/src/test/java/kafka/testkit/KafkaClusterTestKit.java: ## @@ -353,6 +354,9 @@ static private void setupNodeDirectories(File baseDirectory, private final TestKitNodes nodes; private final Map controllers; private final Map brokers; + +private final Map aliveBrokers; Review Comment: > if a broker already shutdown, how can we access the server and check its status? both zk and kraft broker has in-memory state, so maybe you can leverage them? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
chia7712 commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1632358211
##
core/src/test/java/kafka/testkit/KafkaClusterTestKit.java:
##
@@ -188,12 +188,13 @@ private KafkaConfig createNodeConfig(TestKitNode node) {
controllerNode.metadataDirectory());
}
props.put(SocketServerConfigs.LISTENER_SECURITY_PROTOCOL_MAP_CONFIG,
-"EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT");
+
"EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT,PLAINTEXT:PLAINTEXT");
Review Comment:
not sure why we need to call that test helper?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
TaiJuWu commented on code in PR #16127: URL: https://github.com/apache/kafka/pull/16127#discussion_r1632204289 ## core/src/test/java/kafka/testkit/KafkaClusterTestKit.java: ## @@ -353,6 +354,9 @@ static private void setupNodeDirectories(File baseDirectory, private final TestKitNodes nodes; private final Map controllers; private final Map brokers; + +private final Map aliveBrokers; Review Comment: Sounds great but if a broker already shutdown, how can we access the server and check its status? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
TaiJuWu commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1632203912
##
core/src/test/java/kafka/testkit/KafkaClusterTestKit.java:
##
@@ -188,12 +188,13 @@ private KafkaConfig createNodeConfig(TestKitNode node) {
controllerNode.metadataDirectory());
}
props.put(SocketServerConfigs.LISTENER_SECURITY_PROTOCOL_MAP_CONFIG,
-"EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT");
+
"EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT,PLAINTEXT:PLAINTEXT");
Review Comment:
The TestUtils requires sever use `PLAINTEXT` protocol, you can reference
[here.](https://github.com/apache/kafka/blob/d6cd83e2fb2bab4526f07e067277b34e482f6678/core/src/test/scala/unit/kafka/utils/TestUtils.scala#L232)
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] MINOR:Topic command integration test migrate to new test infra [kafka]
chia7712 commented on code in PR #16127:
URL: https://github.com/apache/kafka/pull/16127#discussion_r1632120413
##
core/src/test/java/kafka/testkit/KafkaClusterTestKit.java:
##
@@ -353,6 +354,9 @@ static private void setupNodeDirectories(File baseDirectory,
private final TestKitNodes nodes;
private final Map controllers;
private final Map brokers;
+
+private final Map aliveBrokers;
Review Comment:
Instead of tracing alive brokers, maybe we should add a new method
`isShutdown` to `KafkaBroker`?
##
core/src/test/java/kafka/testkit/KafkaClusterTestKit.java:
##
@@ -188,12 +188,13 @@ private KafkaConfig createNodeConfig(TestKitNode node) {
controllerNode.metadataDirectory());
}
props.put(SocketServerConfigs.LISTENER_SECURITY_PROTOCOL_MAP_CONFIG,
-"EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT");
+
"EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT,PLAINTEXT:PLAINTEXT");
Review Comment:
why we need this change?
--
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
