You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "gaurav-narula (via GitHub)" <gi...@apache.org> on 2024/04/16 11:01:12 UTC
[PR] KAFKA-16559: allow defining number of disks per broker in ClusterTest [kafka]
gaurav-narula opened a new pull request, #15730:
URL: https://github.com/apache/kafka/pull/15730
Overloads `TestKitNodes.Builder::setNumBrokerNodes` to accept a `disksPerBroker` argument. We keep the existing semantics for the case when `disksPerBroker == 1`.
### 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
Re: [PR] KAFKA-16559: allow defining number of disks per broker in ClusterTest [kafka]
Posted by "gaurav-narula (via GitHub)" <gi...@apache.org>.
gaurav-narula commented on PR #15730:
URL: https://github.com/apache/kafka/pull/15730#issuecomment-2058820310
CC: @chia7712 @soarez
--
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
Re: [PR] KAFKA-16559: allow defining number of disks per broker in ClusterTest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #15730:
URL: https://github.com/apache/kafka/pull/15730#discussion_r1567700594
##########
core/src/test/java/kafka/testkit/KafkaClusterTestKitTest.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package kafka.testkit;
+
+import org.hamcrest.Matcher;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import java.nio.file.Paths;
+import java.util.stream.Collectors;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.fail;
+
+public class KafkaClusterTestKitTest {
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void testCreateClusterAndCloseWithMultipleLogDirs(boolean combined) {
+ try (KafkaClusterTestKit cluster = new KafkaClusterTestKit.Builder(
+ new TestKitNodes.Builder().
+ setBrokerNodes(5, 2).
+ setCombined(combined).
+ setNumControllerNodes(3).build()).build()) {
+
+ assertEquals(5, cluster.nodes().brokerNodes().size());
+ assertEquals(3, cluster.nodes().controllerNodes().size());
+
+ cluster.nodes().brokerNodes().forEach((brokerId, node) -> {
+ assertEquals(2, node.logDataDirectories().size());
+ Matcher<Iterable<? extends String>> matcher = containsInAnyOrder(String.format("broker_%d_data0", brokerId), String.format("broker_%d_data1", brokerId));
Review Comment:
> is there any reason not to use hamcrest in core when we do use it in other modules?
I'm curious. Maybe our modules have different tastes of code style :smile:
##########
core/src/test/java/kafka/testkit/KafkaClusterTestKitTest.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package kafka.testkit;
+
+import org.hamcrest.Matcher;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import java.nio.file.Paths;
+import java.util.stream.Collectors;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.fail;
+
+public class KafkaClusterTestKitTest {
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void testCreateClusterAndCloseWithMultipleLogDirs(boolean combined) {
+ try (KafkaClusterTestKit cluster = new KafkaClusterTestKit.Builder(
+ new TestKitNodes.Builder().
+ setBrokerNodes(5, 2).
+ setCombined(combined).
+ setNumControllerNodes(3).build()).build()) {
+
+ assertEquals(5, cluster.nodes().brokerNodes().size());
+ assertEquals(3, cluster.nodes().controllerNodes().size());
+
+ cluster.nodes().brokerNodes().forEach((brokerId, node) -> {
+ assertEquals(2, node.logDataDirectories().size());
+ Matcher<Iterable<? extends String>> matcher = containsInAnyOrder(String.format("broker_%d_data0", brokerId), String.format("broker_%d_data1", brokerId));
Review Comment:
> is there any reason not to use hamcrest in core when we do use it in other modules?
I'm curious too. Maybe our modules have different tastes of code style :smile:
--
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
Re: [PR] KAFKA-16559: allow defining number of disks per broker in ClusterTest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #15730:
URL: https://github.com/apache/kafka/pull/15730#discussion_r1567674070
##########
core/src/test/java/kafka/testkit/KafkaClusterTestKitTest.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package kafka.testkit;
+
+import org.hamcrest.Matcher;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import java.nio.file.Paths;
+import java.util.stream.Collectors;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.fail;
+
+public class KafkaClusterTestKitTest {
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void testCreateClusterAndCloseWithMultipleLogDirs(boolean combined) {
+ try (KafkaClusterTestKit cluster = new KafkaClusterTestKit.Builder(
+ new TestKitNodes.Builder().
+ setBrokerNodes(5, 2).
+ setCombined(combined).
+ setNumControllerNodes(3).build()).build()) {
+
+ assertEquals(5, cluster.nodes().brokerNodes().size());
+ assertEquals(3, cluster.nodes().controllerNodes().size());
+
+ cluster.nodes().brokerNodes().forEach((brokerId, node) -> {
+ assertEquals(2, node.logDataDirectories().size());
+ Matcher<Iterable<? extends String>> matcher = containsInAnyOrder(String.format("broker_%d_data0", brokerId), String.format("broker_%d_data1", brokerId));
Review Comment:
It seems we don't use `hamcrest` in core module. Maybe we can put expected/actual folders into a `HashSet`, and then call `assertEqual` to compare them?
##########
core/src/test/java/kafka/testkit/TestKitNodes.java:
##########
@@ -79,9 +79,16 @@ public Builder setNumControllerNodes(int numControllerNodes) {
}
public Builder setNumBrokerNodes(int numBrokerNodes) {
+ return setBrokerNodes(numBrokerNodes, 1);
+ }
+
+ public Builder setBrokerNodes(int numBrokerNodes, int disksPerBroker) {
if (numBrokerNodes < 0) {
throw new RuntimeException("Invalid negative value for numBrokerNodes");
}
+ if (disksPerBroker < 0) {
Review Comment:
maybe we should throw exception if the value is equal to 0.
--
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
Re: [PR] KAFKA-16559: allow defining number of disks per broker in TestKitNodes [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #15730:
URL: https://github.com/apache/kafka/pull/15730
--
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
Re: [PR] KAFKA-16559: allow defining number of disks per broker in ClusterTest [kafka]
Posted by "gaurav-narula (via GitHub)" <gi...@apache.org>.
gaurav-narula commented on code in PR #15730:
URL: https://github.com/apache/kafka/pull/15730#discussion_r1567676845
##########
core/src/test/java/kafka/testkit/KafkaClusterTestKitTest.java:
##########
@@ -0,0 +1,68 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package kafka.testkit;
+
+import org.hamcrest.Matcher;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import java.nio.file.Paths;
+import java.util.stream.Collectors;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.fail;
+
+public class KafkaClusterTestKitTest {
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void testCreateClusterAndCloseWithMultipleLogDirs(boolean combined) {
+ try (KafkaClusterTestKit cluster = new KafkaClusterTestKit.Builder(
+ new TestKitNodes.Builder().
+ setBrokerNodes(5, 2).
+ setCombined(combined).
+ setNumControllerNodes(3).build()).build()) {
+
+ assertEquals(5, cluster.nodes().brokerNodes().size());
+ assertEquals(3, cluster.nodes().controllerNodes().size());
+
+ cluster.nodes().brokerNodes().forEach((brokerId, node) -> {
+ assertEquals(2, node.logDataDirectories().size());
+ Matcher<Iterable<? extends String>> matcher = containsInAnyOrder(String.format("broker_%d_data0", brokerId), String.format("broker_%d_data1", brokerId));
Review Comment:
Sure. Out of curiosity, is there any reason not to use hamcrest in core when we do use it in other modules?
--
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
Re: [PR] KAFKA-16559: allow defining number of disks per broker in ClusterTest [kafka]
Posted by "gaurav-narula (via GitHub)" <gi...@apache.org>.
gaurav-narula commented on code in PR #15730:
URL: https://github.com/apache/kafka/pull/15730#discussion_r1567295806
##########
core/src/test/scala/integration/kafka/server/KRaftClusterTest.scala:
##########
@@ -74,7 +74,7 @@ class KRaftClusterTest {
def testCreateClusterAndClose(): Unit = {
val cluster = new KafkaClusterTestKit.Builder(
new TestKitNodes.Builder().
- setNumBrokerNodes(1).
+ setNumBrokerNodes(1, 2).
Review Comment:
I was trying to be cheeky to avoid another unit test :) Will add a separate one
--
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
Re: [PR] KAFKA-16559: allow defining number of disks per broker in ClusterTest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #15730:
URL: https://github.com/apache/kafka/pull/15730#discussion_r1567703623
##########
core/src/test/java/kafka/testkit/KafkaClusterTestKitTest.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package kafka.testkit;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.fail;
+
+public class KafkaClusterTestKitTest {
+ @Test
+ public void testCreateClusterWithNoDisksThrows() {
+ try (KafkaClusterTestKit cluster = new KafkaClusterTestKit.Builder(
Review Comment:
We can use `assertThrows` to rewrite it. For example:
```java
Exception e = assertThrows(Exception.class, () -> new KafkaClusterTestKit.Builder(
new TestKitNodes.Builder().
setBrokerNodes(1, 0).
setNumControllerNodes(1).build()));
assertEquals(RuntimeException.class, e.getClass());
assertEquals("Invalid value for disksPerBroker", e.getMessage());
```
--
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
Re: [PR] KAFKA-16559: allow defining number of disks per broker in ClusterTest [kafka]
Posted by "gaurav-narula (via GitHub)" <gi...@apache.org>.
gaurav-narula commented on code in PR #15730:
URL: https://github.com/apache/kafka/pull/15730#discussion_r1567645489
##########
core/src/test/java/kafka/testkit/TestKitNodes.java:
##########
@@ -90,9 +100,17 @@ public Builder setNumBrokerNodes(int numBrokerNodes) {
if (!brokerNodeBuilders.isEmpty()) {
nextId = brokerNodeBuilders.lastKey() + 1;
}
- brokerNodeBuilders.put(nextId,
- new BrokerNode.Builder().
- setId(nextId));
+ BrokerNode.Builder brokerNodeBuilder = new BrokerNode.Builder().setId(nextId);
+ // Keeping consistent behaviour for existing tests when disksPerBroker == 1
+ if (disksPerBroker > 1) {
+ int brokerId = nextId; // this is because the lambda below requires a final
+ List<String> logDataDirectories = IntStream
+ .range(0, disksPerBroker)
+ .mapToObj(i -> String.format("broker_%d_data%d", brokerId, i))
+ .collect(Collectors.toList());
+ brokerNodeBuilder.setLogDirectories(logDataDirectories);
+ }
Review Comment:
Addressed in [0877408](https://github.com/apache/kafka/pull/15730/commits/0877408378561314afb6300cc756b1d22d12366a).
--
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
Re: [PR] KAFKA-16559: allow defining number of disks per broker in ClusterTest [kafka]
Posted by "soarez (via GitHub)" <gi...@apache.org>.
soarez commented on code in PR #15730:
URL: https://github.com/apache/kafka/pull/15730#discussion_r1567303386
##########
core/src/test/java/kafka/testkit/TestKitNodes.java:
##########
@@ -90,9 +100,17 @@ public Builder setNumBrokerNodes(int numBrokerNodes) {
if (!brokerNodeBuilders.isEmpty()) {
nextId = brokerNodeBuilders.lastKey() + 1;
}
- brokerNodeBuilders.put(nextId,
- new BrokerNode.Builder().
- setId(nextId));
+ BrokerNode.Builder brokerNodeBuilder = new BrokerNode.Builder().setId(nextId);
+ // Keeping consistent behaviour for existing tests when disksPerBroker == 1
+ if (disksPerBroker > 1) {
+ int brokerId = nextId; // this is because the lambda below requires a final
+ List<String> logDataDirectories = IntStream
+ .range(0, disksPerBroker)
+ .mapToObj(i -> String.format("broker_%d_data%d", brokerId, i))
+ .collect(Collectors.toList());
+ brokerNodeBuilder.setLogDirectories(logDataDirectories);
+ }
Review Comment:
Should this be part of `BrokerNode.Builder`, perhaps a new `.setNumLogDirectories()`?
--
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
Re: [PR] KAFKA-16559: allow defining number of disks per broker in ClusterTest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #15730:
URL: https://github.com/apache/kafka/pull/15730#discussion_r1567289057
##########
core/src/test/scala/integration/kafka/server/KRaftClusterTest.scala:
##########
@@ -74,7 +74,7 @@ class KRaftClusterTest {
def testCreateClusterAndClose(): Unit = {
val cluster = new KafkaClusterTestKit.Builder(
new TestKitNodes.Builder().
- setNumBrokerNodes(1).
+ setNumBrokerNodes(1, 2).
Review Comment:
Why we need this change?
##########
core/src/test/java/kafka/testkit/TestKitNodes.java:
##########
@@ -79,9 +82,16 @@ public Builder setNumControllerNodes(int numControllerNodes) {
}
public Builder setNumBrokerNodes(int numBrokerNodes) {
+ return setNumBrokerNodes(numBrokerNodes, 1);
+ }
+
+ public Builder setNumBrokerNodes(int numBrokerNodes, int disksPerBroker) {
Review Comment:
As it accepts another argument now, maybe we should rename it to `setBrokerNodes`
##########
core/src/test/java/kafka/testkit/TestKitNodes.java:
##########
@@ -90,9 +100,17 @@ public Builder setNumBrokerNodes(int numBrokerNodes) {
if (!brokerNodeBuilders.isEmpty()) {
nextId = brokerNodeBuilders.lastKey() + 1;
}
- brokerNodeBuilders.put(nextId,
- new BrokerNode.Builder().
- setId(nextId));
+ BrokerNode.Builder brokerNodeBuilder = new BrokerNode.Builder().setId(nextId);
+ // Keeping consistent behaviour for existing tests when disksPerBroker == 1
+ if (disksPerBroker > 1) {
+ int brokerId = nextId; // this is because the lambda below requires a final
+ List<String> logDataDirectories = IntStream
+ .range(0, disksPerBroker)
+ .mapToObj(i -> String.format("broker_%d_data%d", brokerId, i))
+ .collect(Collectors.toList());
+ brokerNodeBuilder.setLogDirectories(logDataDirectories);
Review Comment:
could you add a ut for 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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16559: allow defining number of disks per broker in ClusterTest [kafka]
Posted by "gaurav-narula (via GitHub)" <gi...@apache.org>.
gaurav-narula commented on code in PR #15730:
URL: https://github.com/apache/kafka/pull/15730#discussion_r1567713128
##########
core/src/test/java/kafka/testkit/KafkaClusterTestKitTest.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package kafka.testkit;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+import java.nio.file.Paths;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.fail;
+
+public class KafkaClusterTestKitTest {
+ @Test
+ public void testCreateClusterWithNoDisksThrows() {
+ try (KafkaClusterTestKit cluster = new KafkaClusterTestKit.Builder(
Review Comment:
👍 Using `assertThrowsExactly` instead to avoid the equality check on `RuntimeException.class`
--
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
Re: [PR] KAFKA-16559: allow defining number of disks per broker in ClusterTest [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #15730:
URL: https://github.com/apache/kafka/pull/15730#discussion_r1567313085
##########
core/src/test/java/kafka/testkit/TestKitNodes.java:
##########
@@ -90,9 +100,17 @@ public Builder setNumBrokerNodes(int numBrokerNodes) {
if (!brokerNodeBuilders.isEmpty()) {
nextId = brokerNodeBuilders.lastKey() + 1;
}
- brokerNodeBuilders.put(nextId,
- new BrokerNode.Builder().
- setId(nextId));
+ BrokerNode.Builder brokerNodeBuilder = new BrokerNode.Builder().setId(nextId);
+ // Keeping consistent behaviour for existing tests when disksPerBroker == 1
+ if (disksPerBroker > 1) {
+ int brokerId = nextId; // this is because the lambda below requires a final
+ List<String> logDataDirectories = IntStream
+ .range(0, disksPerBroker)
+ .mapToObj(i -> String.format("broker_%d_data%d", brokerId, i))
+ .collect(Collectors.toList());
+ brokerNodeBuilder.setLogDirectories(logDataDirectories);
+ }
Review Comment:
noted that we should consider the `combined` mode. see https://github.com/apache/kafka/blob/trunk/core/src/test/java/kafka/testkit/BrokerNode.java#L80
--
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