chia7712 commented on code in PR #15761:
URL: https://github.com/apache/kafka/pull/15761#discussion_r1575290259
##########
core/src/test/java/kafka/testkit/BrokerNode.java:
##########
@@ -66,11 +74,27 @@ public Builder setNumLogDirectories(int numLogDirectories) {
return this;
}
- public BrokerNode build(
- String baseDirectory,
- Uuid clusterId,
- boolean combined
- ) {
+ public Builder setClusterId(Uuid clusterId) {
+ this.clusterId = clusterId;
+ return this;
+ }
+
+ public Builder setBaseDirectory(String baseDirectory) {
+ this.baseDirectory = baseDirectory;
+ return this;
+ }
+
+ public Builder setCombined(boolean combined) {
+ this.combined = combined;
+ return this;
+ }
+
+ public Builder setPropertyOverrides(Map<String, String>
propertyOverrides) {
+ this.propertyOverrides =
Collections.unmodifiableMap(propertyOverrides);
Review Comment:
Maybe we need to do a copy in order to avoid changes produced from outer.
##########
core/src/test/java/kafka/testkit/TestKitNodes.java:
##########
@@ -97,14 +96,31 @@ public Builder setBrokerNodes(int numBrokerNodes, int
disksPerBroker) {
if (!brokerNodeBuilders.isEmpty()) {
nextId = brokerNodeBuilders.lastKey() + 1;
}
- BrokerNode.Builder brokerNodeBuilder = new BrokerNode.Builder()
+ BrokerNode.Builder brokerNodeBuilder = BrokerNode.builder()
.setId(nextId)
.setNumLogDirectories(disksPerBroker);
brokerNodeBuilders.put(nextId, brokerNodeBuilder);
}
return this;
}
+ /**
+ * Set per broker properties overrides, this setter must be invoked
after setBrokerNodes which
+ * setup broker id and broker builder.
+ * @param perBrokerPropertiesOverrides properties to override in each
broker
+ * @return Builder
+ */
+ public Builder setPerBrokerPropertiesOverrides(Map<Integer,
Map<String, String>> perBrokerPropertiesOverrides) {
+ perBrokerPropertiesOverrides.forEach((brokerId, properties) -> {
+ if (!brokerNodeBuilders.containsKey(brokerId)) {
+ throw new RuntimeException("Broker id " + brokerId + "
does not exist");
+ }
+ Map<String, String> propertiesOverride = new
HashMap<>(properties);
Review Comment:
the deep copy should be addressed by `setPropertyOverrides`
##########
core/src/test/java/kafka/testkit/TestKitNodes.java:
##########
@@ -97,14 +96,32 @@ public Builder setBrokerNodes(int numBrokerNodes, int
disksPerBroker) {
if (!brokerNodeBuilders.isEmpty()) {
nextId = brokerNodeBuilders.lastKey() + 1;
}
- BrokerNode.Builder brokerNodeBuilder = new BrokerNode.Builder()
+ BrokerNode.Builder brokerNodeBuilder = BrokerNode.builder()
.setId(nextId)
.setNumLogDirectories(disksPerBroker);
brokerNodeBuilders.put(nextId, brokerNodeBuilder);
}
return this;
}
+ /**
+ * Set per broker properties overrides, this setter must be invoked
after setBrokerNodes which
Review Comment:
It seems `TestKitNodes` need to be refactor also. In short, it should
verify/apply all settings when building.
##########
core/src/test/java/kafka/test/ClusterConfig.java:
##########
@@ -126,8 +138,12 @@ public MetadataVersion metadataVersion() {
return metadataVersion;
}
- public Properties brokerServerProperties(int brokerId) {
- return perBrokerOverrideProperties.computeIfAbsent(brokerId, __ -> new
Properties());
+ public Map<String, String> brokerServerProperties(int brokerId) {
Review Comment:
This helper is used by `ZkClusterInvocationContext` only. Maybe we can
remove this one to simplify `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
