chia7712 commented on code in PR #15745:
URL: https://github.com/apache/kafka/pull/15745#discussion_r1585496531
##########
core/src/test/java/kafka/test/ClusterConfig.java:
##########
@@ -319,7 +302,10 @@ public Builder setPerBrokerProperties(Map<Integer,
Map<String, String>> perBroke
}
public ClusterConfig build() {
- return new ClusterConfig(type, brokers, controllers, name,
autoStart, securityProtocol, listenerName,
+ if (brokers <= 0 || controllers <= 0 || disksPerBroker <= 0) {
Review Comment:
We should enable to set `brokers` to 0 as it is valid to have a cluster with
only quorum nodes.
##########
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##########
@@ -202,8 +151,19 @@ private void processClusterTest(ExtensionContext context,
ClusterTest annot, Clu
for (ClusterConfigProperty property : annot.serverProperties()) {
serverProperties.put(property.key(), property.value());
}
- configBuilder.setServerProperties(serverProperties);
- type.invocationContexts(context.getRequiredTestMethod().getName(),
configBuilder.build(), testInvocations);
+ ClusterConfig config = ClusterConfig.builder()
+ .setType(type)
+ .setBrokers(annot.brokers() == 0 ? defaults.brokers() :
annot.brokers())
+ .setControllers(annot.controllers() == 0 ?
defaults.controllers() : annot.controllers())
+ .setDisksPerBroker(annot.disksPerBroker() == 0 ?
defaults.disksPerBroker() : annot.disksPerBroker())
+ .setAutoStart(annot.autoStart() == AutoStart.DEFAULT ?
defaults.autoStart() : annot.autoStart() == AutoStart.YES)
+ .setName(annot.name().isEmpty() ? null : annot.name())
Review Comment:
maybe `annot.name().trim().isEmpty()` is more suitable.
##########
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##########
@@ -202,8 +151,19 @@ private void processClusterTest(ExtensionContext context,
ClusterTest annot, Clu
for (ClusterConfigProperty property : annot.serverProperties()) {
serverProperties.put(property.key(), property.value());
}
- configBuilder.setServerProperties(serverProperties);
- type.invocationContexts(context.getRequiredTestMethod().getName(),
configBuilder.build(), testInvocations);
+ ClusterConfig config = ClusterConfig.builder()
+ .setType(type)
+ .setBrokers(annot.brokers() == 0 ? defaults.brokers() :
annot.brokers())
+ .setControllers(annot.controllers() == 0 ?
defaults.controllers() : annot.controllers())
+ .setDisksPerBroker(annot.disksPerBroker() == 0 ?
defaults.disksPerBroker() : annot.disksPerBroker())
+ .setAutoStart(annot.autoStart() == AutoStart.DEFAULT ?
defaults.autoStart() : annot.autoStart() == AutoStart.YES)
+ .setName(annot.name().isEmpty() ? null : annot.name())
+ .setListenerName(annot.listener().isEmpty() ? null :
annot.listener())
Review Comment:
ditto
##########
core/src/test/java/kafka/test/ClusterConfig.java:
##########
@@ -319,7 +302,10 @@ public Builder setPerBrokerProperties(Map<Integer,
Map<String, String>> perBroke
}
public ClusterConfig build() {
- return new ClusterConfig(type, brokers, controllers, name,
autoStart, securityProtocol, listenerName,
+ if (brokers <= 0 || controllers <= 0 || disksPerBroker <= 0) {
Review Comment:
btw, those check should be moved to constructor of `ClusterConfig`
--
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
