Re: [PR] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
chia7712 merged PR #15745: URL: https://github.com/apache/kafka/pull/15745 -- 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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
FrankYang0529 commented on code in PR #15745:
URL: https://github.com/apache/kafka/pull/15745#discussion_r158855
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -55,14 +56,21 @@ public class ClusterConfig {
private final Map>
perBrokerOverrideProperties;
@SuppressWarnings("checkstyle:ParameterNumber")
-private ClusterConfig(Type type, int brokers, int controllers, String
name, boolean autoStart,
+private ClusterConfig(Type type, int brokers, int controllers, int
disksPerBroker, String name, boolean autoStart,
SecurityProtocol securityProtocol, String listenerName, File
trustStoreFile,
MetadataVersion metadataVersion, Map
serverProperties, Map producerProperties,
Map consumerProperties, Map
adminClientProperties, Map saslServerProperties,
Map saslClientProperties, Map> perBrokerOverrideProperties) {
+if (brokers < 0) {
+throw new IllegalArgumentException("Number of brokers must be
greater or equal to zero.");
+}
+if (controllers <= 0 || disksPerBroker <= 0) {
Review Comment:
Sorry, I misunderstood your message. Updated it. Thank you.
--
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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
chia7712 commented on code in PR #15745:
URL: https://github.com/apache/kafka/pull/15745#discussion_r1588469427
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -55,14 +56,21 @@ public class ClusterConfig {
private final Map>
perBrokerOverrideProperties;
@SuppressWarnings("checkstyle:ParameterNumber")
-private ClusterConfig(Type type, int brokers, int controllers, String
name, boolean autoStart,
+private ClusterConfig(Type type, int brokers, int controllers, int
disksPerBroker, String name, boolean autoStart,
SecurityProtocol securityProtocol, String listenerName, File
trustStoreFile,
MetadataVersion metadataVersion, Map
serverProperties, Map producerProperties,
Map consumerProperties, Map
adminClientProperties, Map saslServerProperties,
Map saslClientProperties, Map> perBrokerOverrideProperties) {
+if (brokers < 0) {
+throw new IllegalArgumentException("Number of brokers must be
greater or equal to zero.");
+}
+if (controllers <= 0 || disksPerBroker <= 0) {
Review Comment:
My point was "controllers can be zero in zk case", so `ClusterConfig` should
not throw exception. Hence, we should have following checks.
```java
// do fail fast. the following values are invalid for both zk and
kraft modes.
if (brokers < 0) throw new IllegalArgumentException("Number of
brokers must be greater or equal to zero.");
if (controllers < 0) throw new IllegalArgumentException("Number of
controller must be greater or equal to zero.");
if (disksPerBroker <= 0) throw new IllegalArgumentException("Number
of disks must be greater than zero.");
```
--
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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
FrankYang0529 commented on PR #15745: URL: https://github.com/apache/kafka/pull/15745#issuecomment-2090298946 > @FrankYang0529 thanks for updated PR. two comments left. Hi @chia7712, thanks for the review. I addressed last comments. -- 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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
chia7712 commented on code in PR #15745:
URL: https://github.com/apache/kafka/pull/15745#discussion_r1587006765
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -55,14 +56,21 @@ public class ClusterConfig {
private final Map>
perBrokerOverrideProperties;
@SuppressWarnings("checkstyle:ParameterNumber")
-private ClusterConfig(Type type, int brokers, int controllers, String
name, boolean autoStart,
+private ClusterConfig(Type type, int brokers, int controllers, int
disksPerBroker, String name, boolean autoStart,
SecurityProtocol securityProtocol, String listenerName, File
trustStoreFile,
MetadataVersion metadataVersion, Map
serverProperties, Map producerProperties,
Map consumerProperties, Map
adminClientProperties, Map saslServerProperties,
Map saslClientProperties, Map> perBrokerOverrideProperties) {
+if (brokers < 0) {
+throw new IllegalArgumentException("Number of brokers must be
greater or equal to zero.");
+}
+if (controllers <= 0 || disksPerBroker <= 0) {
Review Comment:
`controllers <= 0` is acceptable if it is zk mode. Also, `TestKitNodes`
guards against that already
https://github.com/apache/kafka/blob/89d8045a15b622805f65c3c6fbfde82606921f65/core/src/test/java/kafka/testkit/TestKitNodes.java#L90
Hence, `ClusterConfig` does not require such check, and we can add comment
to explain why we don't need to check `controllers` here.
##
core/src/test/java/kafka/test/ClusterTestExtensionsTest.java:
##
@@ -104,6 +107,26 @@ public void testClusterTests() {
}
}
+@ClusterTests({
+@ClusterTest(clusterType = Type.ZK),
Review Comment:
We should test all types for both cases: "default" + "custom"
--
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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
FrankYang0529 commented on PR #15745: URL: https://github.com/apache/kafka/pull/15745#issuecomment-2087904101 Hi @chia7712, thanks for the review. I address all comments and add some test cases for 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
Re: [PR] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
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> 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> 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
Re: [PR] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
FrankYang0529 commented on code in PR #15745:
URL: https://github.com/apache/kafka/pull/15745#discussion_r1584379526
##
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##
@@ -162,32 +162,39 @@ private void processClusterTest(ExtensionContext context,
ClusterTest annot, Clu
controllers = annot.controllers();
}
-if (brokers <= 0 || controllers <= 0) {
-throw new IllegalArgumentException("Number of brokers/controllers
must be greater than zero.");
+final int disksPerBroker;
+if (annot.disksPerBroker() == 0) {
+disksPerBroker = defaults.disksPerBroker();
Review Comment:
Thanks for the suggestion. It's more clear. Updated 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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
chia7712 commented on code in PR #15745:
URL: https://github.com/apache/kafka/pull/15745#discussion_r1583452740
##
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##
@@ -162,32 +162,39 @@ private void processClusterTest(ExtensionContext context,
ClusterTest annot, Clu
controllers = annot.controllers();
}
-if (brokers <= 0 || controllers <= 0) {
-throw new IllegalArgumentException("Number of brokers/controllers
must be greater than zero.");
+final int disksPerBroker;
+if (annot.disksPerBroker() == 0) {
+disksPerBroker = defaults.disksPerBroker();
Review Comment:
We can leverage ternary conditional operator to make it more simple. For
example:
```java
Type type = annot.clusterType() == Type.DEFAULT ?
defaults.clusterType() : annot.clusterType();
Map serverProperties = new HashMap<>();
for (ClusterConfigProperty property : defaults.serverProperties()) {
serverProperties.put(property.key(), property.value());
}
for (ClusterConfigProperty property : annot.serverProperties()) {
serverProperties.put(property.key(), property.value());
}
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())
.setName(annot.listener().isEmpty() ? null : annot.listener())
.setServerProperties(serverProperties)
.setSecurityProtocol(annot.securityProtocol())
.setMetadataVersion(annot.metadataVersion())
.build();
type.invocationContexts(context.getRequiredTestMethod().getName(),
config, testInvocations);
```
--
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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
FrankYang0529 commented on code in PR #15745:
URL: https://github.com/apache/kafka/pull/15745#discussion_r1582798499
##
core/src/test/java/kafka/test/ClusterTestExtensionsTest.java:
##
@@ -121,6 +124,26 @@ public void testClusterTests() {
}
}
+@ClusterTests({
+@ClusterTest(clusterType = Type.ZK, disksPerBroker = 1),
Review Comment:
Thanks for finding this. Remove 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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
FrankYang0529 commented on code in PR #15745:
URL: https://github.com/apache/kafka/pull/15745#discussion_r1582798251
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -55,14 +56,15 @@ public class ClusterConfig {
private final Map>
perBrokerOverrideProperties;
@SuppressWarnings("checkstyle:ParameterNumber")
-private ClusterConfig(Type type, int brokers, int controllers, String
name, boolean autoStart,
+private ClusterConfig(Type type, int brokers, int controllers, int
disksPerBroker, String name, boolean autoStart,
SecurityProtocol securityProtocol, String listenerName, File
trustStoreFile,
MetadataVersion metadataVersion, Map
serverProperties, Map producerProperties,
Map consumerProperties, Map
adminClientProperties, Map saslServerProperties,
Map saslClientProperties, Map> perBrokerOverrideProperties) {
this.type = Objects.requireNonNull(type);
this.brokers = brokers;
this.controllers = controllers;
+this.disksPerBroker = disksPerBroker;
Review Comment:
I add check in `ClusterConfig#Builder#build` 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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
FrankYang0529 commented on code in PR #15745:
URL: https://github.com/apache/kafka/pull/15745#discussion_r1582797729
##
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##
@@ -162,32 +162,39 @@ private void processClusterTest(ExtensionContext context,
ClusterTest annot, Clu
controllers = annot.controllers();
}
-if (brokers <= 0 || controllers <= 0) {
-throw new IllegalArgumentException("Number of brokers/controllers
must be greater than zero.");
+final int disksPerBroker;
+if (annot.disksPerBroker() == 0) {
+disksPerBroker = defaults.disksPerBroker();
Review Comment:
Yeah, I remove `brokers`, `controllers`, and `disksPerBroker`, but keeping
`type`. We can't get attribute from `ClusterConfig#Builder` and we need `type`
to run `invocationContexts`.
--
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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
FrankYang0529 commented on code in PR #15745:
URL: https://github.com/apache/kafka/pull/15745#discussion_r1582795316
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -160,6 +166,7 @@ public boolean equals(Object object) {
return Objects.equals(type, clusterConfig.type)
&& Objects.equals(brokers, clusterConfig.brokers)
&& Objects.equals(controllers, clusterConfig.controllers)
+&& Objects.equals(disksPerBroker, clusterConfig.disksPerBroker)
Review Comment:
Remove `hashCode` and `equals` functions. Thanks.
--
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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
brandboat commented on code in PR #15745:
URL: https://github.com/apache/kafka/pull/15745#discussion_r1582482314
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -160,6 +166,7 @@ public boolean equals(Object object) {
return Objects.equals(type, clusterConfig.type)
&& Objects.equals(brokers, clusterConfig.brokers)
&& Objects.equals(controllers, clusterConfig.controllers)
+&& Objects.equals(disksPerBroker, clusterConfig.disksPerBroker)
Review Comment:
feel free to remove them as currently they are unused 😃
--
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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
chia7712 commented on code in PR #15745:
URL: https://github.com/apache/kafka/pull/15745#discussion_r1582250475
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -55,14 +56,15 @@ public class ClusterConfig {
private final Map>
perBrokerOverrideProperties;
@SuppressWarnings("checkstyle:ParameterNumber")
-private ClusterConfig(Type type, int brokers, int controllers, String
name, boolean autoStart,
+private ClusterConfig(Type type, int brokers, int controllers, int
disksPerBroker, String name, boolean autoStart,
SecurityProtocol securityProtocol, String listenerName, File
trustStoreFile,
MetadataVersion metadataVersion, Map
serverProperties, Map producerProperties,
Map consumerProperties, Map
adminClientProperties, Map saslServerProperties,
Map saslClientProperties, Map> perBrokerOverrideProperties) {
this.type = Objects.requireNonNull(type);
this.brokers = brokers;
this.controllers = controllers;
+this.disksPerBroker = disksPerBroker;
Review Comment:
Could you add check to make sure the value is larger than zero?
##
core/src/test/java/kafka/test/ClusterTestExtensionsTest.java:
##
@@ -121,6 +124,26 @@ public void testClusterTests() {
}
}
+@ClusterTests({
+@ClusterTest(clusterType = Type.ZK, disksPerBroker = 1),
Review Comment:
We can't use `disksPerBroker = 1` since it is equal to the default value.
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -160,6 +166,7 @@ public boolean equals(Object object) {
return Objects.equals(type, clusterConfig.type)
&& Objects.equals(brokers, clusterConfig.brokers)
&& Objects.equals(controllers, clusterConfig.controllers)
+&& Objects.equals(disksPerBroker, clusterConfig.disksPerBroker)
Review Comment:
not sure whether we need to keep `hashCode` and `equals`. We don't use it
actually and it is error-prone when adding new fields. For example, this PR
does not update `hashCode` for the new field. @brandboat WDTY?
##
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##
@@ -162,32 +162,39 @@ private void processClusterTest(ExtensionContext context,
ClusterTest annot, Clu
controllers = annot.controllers();
}
-if (brokers <= 0 || controllers <= 0) {
-throw new IllegalArgumentException("Number of brokers/controllers
must be greater than zero.");
+final int disksPerBroker;
+if (annot.disksPerBroker() == 0) {
+disksPerBroker = defaults.disksPerBroker();
Review Comment:
Could we leverage the builder instead of creating a bunch of temporary local
variables?
--
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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
FrankYang0529 commented on PR #15745: URL: https://github.com/apache/kafka/pull/15745#issuecomment-2081484120 > @FrankYang0529 please fix the conflicts Hi @chia7712, I resolve conflicts. Thank you. -- 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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
chia7712 commented on PR #15745: URL: https://github.com/apache/kafka/pull/15745#issuecomment-2081116673 @FrankYang0529 please fix the conflicts -- 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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
chia7712 commented on PR #15745: URL: https://github.com/apache/kafka/pull/15745#issuecomment-2069589870 @FrankYang0529 This PR will enhance our new test infra. However, @brandboat is working at refactoring the infra at the same time. It seems to me this PR should be blocked by #15761 in order to avoid big conflicts. Also, this PR can do the enhancement based on the new graceful code base. -- 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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
FrankYang0529 commented on code in PR #15745: URL: https://github.com/apache/kafka/pull/15745#discussion_r1572189143 ## core/src/test/java/kafka/test/junit/ClusterTestExtensions.java: ## @@ -179,8 +186,8 @@ private void processClusterTest(ExtensionContext context, ClusterTest annot, Clu throw new IllegalStateException(); } -ClusterConfig.Builder builder = ClusterConfig.clusterBuilder(type, brokers, controllers, autoStart, -annot.securityProtocol(), annot.metadataVersion()); +ClusterConfig.Builder builder = ClusterConfig.clusterBuilder(type, brokers, controllers, disksPerBroker, Review Comment: Thanks for the suggestion. I updated 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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
chia7712 commented on code in PR #15745: URL: https://github.com/apache/kafka/pull/15745#discussion_r1572122943 ## core/src/test/java/kafka/test/junit/ClusterTestExtensions.java: ## @@ -179,8 +186,8 @@ private void processClusterTest(ExtensionContext context, ClusterTest annot, Clu throw new IllegalStateException(); } -ClusterConfig.Builder builder = ClusterConfig.clusterBuilder(type, brokers, controllers, autoStart, -annot.securityProtocol(), annot.metadataVersion()); +ClusterConfig.Builder builder = ClusterConfig.clusterBuilder(type, brokers, controllers, disksPerBroker, Review Comment: We can do that in this PR as most changes involved by refactor are in this method. Let's do it to decorate our test infrastructure:) -- 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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
FrankYang0529 commented on code in PR #15745: URL: https://github.com/apache/kafka/pull/15745#discussion_r1572114481 ## core/src/test/java/kafka/test/junit/ClusterTestExtensions.java: ## @@ -179,8 +186,8 @@ private void processClusterTest(ExtensionContext context, ClusterTest annot, Clu throw new IllegalStateException(); } -ClusterConfig.Builder builder = ClusterConfig.clusterBuilder(type, brokers, controllers, autoStart, -annot.securityProtocol(), annot.metadataVersion()); +ClusterConfig.Builder builder = ClusterConfig.clusterBuilder(type, brokers, controllers, disksPerBroker, Review Comment: Do we want to use another jira to trace this? Or I remove it in this PR? Thanks. -- 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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
chia7712 commented on code in PR #15745:
URL: https://github.com/apache/kafka/pull/15745#discussion_r1572064864
##
core/src/test/java/kafka/test/junit/ClusterTestExtensions.java:
##
@@ -179,8 +186,8 @@ private void processClusterTest(ExtensionContext context,
ClusterTest annot, Clu
throw new IllegalStateException();
}
-ClusterConfig.Builder builder = ClusterConfig.clusterBuilder(type,
brokers, controllers, autoStart,
-annot.securityProtocol(), annot.metadataVersion());
+ClusterConfig.Builder builder = ClusterConfig.clusterBuilder(type,
brokers, controllers, disksPerBroker,
Review Comment:
It seems to me `clusterBuilder` should be removed. Also, the temporary local
variables (`type`, `brokers`, etc ) can be removed. We have a builder already
and hence we should leverage it to collect all variables. for example:
```java
ClusterConfig.Builder builder = ClusterConfig.builder();
if (annot.clusterType() == Type.DEFAULT) {
builder.type(defaults.clusterType());
} else {
builder.type(annot.clusterType());
}
```
--
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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
FrankYang0529 commented on PR #15745: URL: https://github.com/apache/kafka/pull/15745#issuecomment-2066132584 Hi @gaurav-narula and @chia7712, I have addressed all comments. 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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
chia7712 commented on code in PR #15745:
URL: https://github.com/apache/kafka/pull/15745#discussion_r1569310965
##
core/src/test/java/kafka/testkit/TestKitNodes.java:
##
@@ -82,6 +82,10 @@ public Builder setNumBrokerNodes(int numBrokerNodes) {
return setBrokerNodes(numBrokerNodes, 1);
}
+public Builder setNumBrokerNodes(int numBrokerNodes, int
disksPerBroker) {
Review Comment:
why we need this method? it is equal to `setBrokerNodes`
##
core/src/test/java/kafka/test/ClusterConfig.java:
##
@@ -155,29 +161,31 @@ public ClusterConfig copyOf() {
}
public static Builder defaultClusterBuilder() {
-return new Builder(Type.ZK, 1, 1, true, SecurityProtocol.PLAINTEXT,
MetadataVersion.latestTesting());
+return new Builder(Type.ZK, 1, 1, 1, true, SecurityProtocol.PLAINTEXT,
MetadataVersion.latestTesting());
}
-public static Builder clusterBuilder(Type type, int brokers, int
controllers, boolean autoStart,
+public static Builder clusterBuilder(Type type, int brokers, int
controllers, int disksPerBroker, boolean autoStart,
SecurityProtocol securityProtocol,
MetadataVersion metadataVersion) {
-return new Builder(type, brokers, controllers, autoStart,
securityProtocol, metadataVersion);
+return new Builder(type, brokers, controllers, disksPerBroker,
autoStart, securityProtocol, metadataVersion);
}
public static class Builder {
private Type type;
private int brokers;
private int controllers;
+private int disksPerBroker;
private String name;
private boolean autoStart;
private SecurityProtocol securityProtocol;
private String listenerName;
private File trustStoreFile;
private MetadataVersion metadataVersion;
-Builder(Type type, int brokers, int controllers, boolean autoStart,
SecurityProtocol securityProtocol, MetadataVersion metadataVersion) {
+Builder(Type type, int brokers, int controllers, int disksPerBroker,
boolean autoStart, SecurityProtocol securityProtocol, MetadataVersion
metadataVersion) {
Review Comment:
Could we call setters instead of having a complicated constructor? We should
not waste the build pattern ... this is unrelated to this PR, and it already
have a jira (https://issues.apache.org/jira/browse/KAFKA-16560). However, we
can do a bit refactor here.
--
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] KAFKA-16572: allow defining number of disks per broker in ClusterTest [kafka]
gaurav-narula commented on code in PR #15745:
URL: https://github.com/apache/kafka/pull/15745#discussion_r1569160539
##
core/src/test/java/kafka/test/junit/RaftClusterInvocationContext.java:
##
@@ -93,7 +93,7 @@ public List getAdditionalExtensions() {
TestKitNodes nodes = new TestKitNodes.Builder().
setBootstrapMetadataVersion(clusterConfig.metadataVersion()).
setCombined(isCombined).
-setNumBrokerNodes(clusterConfig.numBrokers()).
+setNumBrokerNodes(clusterConfig.numBrokers(),
clusterConfig.numDisksPerBroker()).
Review Comment:
```suggestion
setBrokerNodes(clusterConfig.numBrokers(),
clusterConfig.numDisksPerBroker()).
```
##
core/src/test/java/kafka/test/ClusterTestExtensionsTest.java:
##
@@ -114,6 +118,25 @@ public void testClusterTests() {
}
}
+@ClusterTests({
+@ClusterTest(name = "cluster-tests-1", clusterType = Type.ZK,
disksPerBroker = 1),
+@ClusterTest(name = "cluster-tests-2", clusterType = Type.KRAFT,
disksPerBroker = 2),
+@ClusterTest(name = "cluster-tests-3", clusterType = Type.CO_KRAFT,
disksPerBroker = 2)
+})
+public void testClusterTestWithDisksPerBroker() throws ExecutionException,
InterruptedException {
+Admin admin = clusterInstance.createAdminClient();
+
+DescribeLogDirsResult result =
admin.describeLogDirs(clusterInstance.brokerIds());
+result.allDescriptions().get().forEach((brokerId,
logDirDescriptionMap) -> {
+if
(clusterInstance.clusterType().equals(ClusterInstance.ClusterType.ZK)) {
+Assertions.assertEquals(1, logDirDescriptionMap.size());
+} else if
(clusterInstance.clusterType().equals(ClusterInstance.ClusterType.RAFT)) {
+Assertions.assertEquals(2, logDirDescriptionMap.size());
+} else {
+Assertions.fail("Unknown cluster type " +
clusterInstance.clusterType());
+}
+});
+}
Review Comment:
Nit: new line after the 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
