chia7712 commented on code in PR #15761:
URL: https://github.com/apache/kafka/pull/15761#discussion_r1576155040
##########
core/src/test/java/kafka/testkit/TestKitNodes.java:
##########
@@ -167,11 +186,11 @@ private TestKitNodes(
NavigableMap<Integer, ControllerNode> controllerNodes,
NavigableMap<Integer, BrokerNode> brokerNodes
) {
- this.baseDirectory = baseDirectory;
- this.clusterId = clusterId;
- this.bootstrapMetadata = bootstrapMetadata;
- this.controllerNodes = controllerNodes;
- this.brokerNodes = brokerNodes;
+ this.baseDirectory = Objects.requireNonNull(baseDirectory);
+ this.clusterId = Objects.requireNonNull(clusterId);
+ this.bootstrapMetadata = Objects.requireNonNull(bootstrapMetadata);
+ this.controllerNodes = Objects.requireNonNull(controllerNodes);
Review Comment:
Please follow the rule of this PR - be a immutable object
##########
core/src/test/java/kafka/testkit/BrokerNode.java:
##########
@@ -121,16 +146,16 @@ public BrokerNode build(
private final boolean combined;
private final Map<String, String> propertyOverrides;
- BrokerNode(
+ private BrokerNode(
Uuid incarnationId,
MetaPropertiesEnsemble initialMetaPropertiesEnsemble,
boolean combined,
Map<String, String> propertyOverrides
) {
- this.incarnationId = incarnationId;
- this.initialMetaPropertiesEnsemble = initialMetaPropertiesEnsemble;
+ this.incarnationId = Objects.requireNonNull(incarnationId);
Review Comment:
this is not used so it is fine to remove it
##########
core/src/test/java/kafka/testkit/TestKitNodes.java:
##########
@@ -97,9 +107,12 @@ public Builder setBrokerNodes(int numBrokerNodes, int
disksPerBroker) {
if (!brokerNodeBuilders.isEmpty()) {
nextId = brokerNodeBuilders.lastKey() + 1;
}
- BrokerNode.Builder brokerNodeBuilder = new BrokerNode.Builder()
Review Comment:
Can we evaluate those settings in `build` method? That can simplify code
since we don't need to revert the changes (for example, the number of brokers).
##########
core/src/test/java/kafka/testkit/BrokerNode.java:
##########
@@ -121,16 +146,16 @@ public BrokerNode build(
private final boolean combined;
private final Map<String, String> propertyOverrides;
- BrokerNode(
+ private BrokerNode(
Uuid incarnationId,
MetaPropertiesEnsemble initialMetaPropertiesEnsemble,
boolean combined,
Map<String, String> propertyOverrides
) {
- this.incarnationId = incarnationId;
- this.initialMetaPropertiesEnsemble = initialMetaPropertiesEnsemble;
Review Comment:
`logDataDirectories` should return immutable collection
--
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
