cassandra-pr mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <>
Subject [GitHub] [cassandra] dcapwell commented on a change in pull request #539: CASSANDRA-15733 jvm dtest builder should be provided to the factory and expose state
Date Thu, 16 Apr 2020 20:53:14 GMT
dcapwell commented on a change in pull request #539: CASSANDRA-15733 jvm dtest builder should
be provided to the factory and expose state

 File path: test/distributed/org/apache/cassandra/distributed/impl/
 @@ -241,26 +249,53 @@ public void uncaughtException(Thread thread, Throwable throwable)
-    protected AbstractCluster(File root, Versions.Version initialVersion, List<IInstanceConfig>
-                              ClassLoader sharedClassLoader)
+    protected AbstractCluster(AbstractBuilder builder)
-        this.root = root;
-        this.sharedClassLoader = sharedClassLoader;
+        this.root = builder.getRoot();
+        this.sharedClassLoader = builder.getSharedClassLoader();
+        this.subnet = builder.getSubnet();
+        this.tokenSupplier = builder.getTokenSupplier();
+        this.nodeIdTopology = builder.getNodeIdTopology();
+        this.configUpdater = builder.getConfigUpdater();
         this.instances = new ArrayList<>();
         this.instanceMap = new HashMap<>();
-        this.initialVersion = initialVersion;
-        int generation = AbstractCluster.generation.incrementAndGet();
+        this.initialVersion = builder.getVersion();
+        this.filters = new MessageFilters();
-        for (IInstanceConfig config : configs)
+        int generation = AbstractCluster.generation.incrementAndGet();
+        for (int i = 0; i < builder.getNodeCount(); ++i)
+            int nodeNum = i + 1;
+            InstanceConfig config = createInstanceConfig(nodeNum);
             I instance = newInstanceWrapperInternal(generation, initialVersion, config);
             // we use the config().broadcastAddressAndPort() here because we have not initialised
the Instance
             I prev = instanceMap.put(instance.broadcastAddress(), instance);
             if (null != prev)
                 throw new IllegalStateException("Cluster cannot have multiple nodes with
same InetAddressAndPort: " + instance.broadcastAddress() + " vs " + prev.broadcastAddress());
-        this.filters = new MessageFilters();
+    }
+    public InstanceConfig newInstanceConfig()
+    {
+        return createInstanceConfig(size() + 1);
+    }
+    private InstanceConfig createInstanceConfig(int nodeNum)
 Review comment:
   two things: "leave createInstanceConfig in Builder" and "we'll inevitably duplicate 4 times."
   "leave createInstanceConfig in Builder"
   I think the builder should be simple and small else it can get in the way.  It also gets
ambiguous to semantics as in the case of the bootstrap test.  The cluster was created with
one set of configs, the node is created with a different setup (maybe for a different cluster);
this means its easy to use the API incorrectly.  It is simpler to reason about if the builder
is just that, a set of methods to aid in calling `new` on the cluster class.
   "we'll inevitably duplicate 4 times."
   May make sense to have a utility function that provides this, you can choose to use it
or not but its there to replace the need to write multiple times. And if you need to fork,
nothing is getting in your way.
   So I am fine with a utility method that has the logic, but don't think it should live in
the Builder

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.
For queries about this service, please contact Infrastructure at:

With regards,
Apache Git Services

To unsubscribe, e-mail:
For additional commands, e-mail:

View raw message