cmccabe commented on code in PR #14678: URL: https://github.com/apache/kafka/pull/14678#discussion_r1380540025
########## core/src/test/java/kafka/testkit/TestKitNodes.java: ########## @@ -119,7 +101,35 @@ public TestKitNodes build() { if (bootstrapMetadataVersion == null) { bootstrapMetadataVersion = MetadataVersion.latest(); } - return new TestKitNodes(clusterId, bootstrapMetadataVersion, controllerNodes, brokerNodes); + String baseDirectory = TestUtils.tempDirectory("kafka_" + clusterId).getAbsolutePath(); + try { + NavigableMap<Integer, ControllerNode> controllerNodes = new TreeMap<>(); + for (ControllerNode.Builder controllerNodeBuilder : controllerNodeBuilders.values()) { + ControllerNode controllerNode = controllerNodeBuilder.build(baseDirectory); + if (controllerNodes.put(controllerNode.id(), controllerNode) != null) { + throw new RuntimeException("More than one controller claimed ID " + controllerNode.id()); + } + } + NavigableMap<Integer, BrokerNode> brokerNodes = new TreeMap<>(); + for (BrokerNode.Builder brokerNodeBuilder : brokerNodeBuilders.values()) { + BrokerNode brokerNode = brokerNodeBuilder.build(baseDirectory); + if (brokerNodes.put(brokerNode.id(), brokerNode) != null) { + throw new RuntimeException("More than one broker claimed ID " + brokerNode.id()); + } + } + return new TestKitNodes(baseDirectory, + clusterId, + bootstrapMetadataVersion, + controllerNodes, + brokerNodes); + } catch (Exception e) { Review Comment: Yeah, that's fair. We can only have `RuntimException` here. Still, I can see no logical reason why we wouldn't do the deletion if the code was restructured to throw checked exceptions here. So it seems clearer this way. -- 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