ivanzlenko commented on code in PR #6774:
URL: https://github.com/apache/ignite-3/pull/6774#discussion_r2431423395
##########
modules/compatibility-tests/src/testFixtures/java/org/apache/ignite/internal/IgniteCluster.java:
##########
@@ -88,7 +88,7 @@ public class IgniteCluster {
private final HttpClient client = HttpClient.newBuilder().build();
// External process nodes
- private List<RunnerNode> runnerNodes;
+ private final List<RunnerNode> runnerNodes =
Collections.synchronizedList(new ArrayList<>());
Review Comment:
Let's use CopyOnWriteArrayList please
##########
modules/compatibility-tests/src/testFixtures/java/org/apache/ignite/internal/IgniteCluster.java:
##########
@@ -74,8 +74,8 @@
import org.junit.jupiter.api.TestInfo;
/**
- * Cluster of nodes. Can be started with nodes of previous Ignite versions
running in the external processes or in the embedded mode
Review Comment:
Change not related to PR
##########
modules/compatibility-tests/src/testFixtures/java/org/apache/ignite/internal/IgniteCluster.java:
##########
@@ -173,18 +180,17 @@ public void stop() {
LOG.info("Shut the embedded cluster down");
- if (runnerNodes != null) {
- List<String> nodeNames = runnerNodes.stream()
- .map(RunnerNode::nodeName)
- .collect(toList());
+ List<String> nodeNames = runnerNodes.stream()
+ .filter(Objects::nonNull)
+ .map(RunnerNode::nodeName)
+ .collect(toList());
- LOG.info("Shutting the runner nodes down: [nodes={}]", nodeNames);
+ LOG.info("Shutting the runner nodes down: [nodes={}]", nodeNames);
- runnerNodes.parallelStream().forEach(RunnerNode::stop);
- runnerNodes.clear();
+
runnerNodes.parallelStream().filter(Objects::nonNull).forEach(RunnerNode::stop);
Review Comment:
Same
##########
modules/compatibility-tests/src/testFixtures/java/org/apache/ignite/internal/IgniteCluster.java:
##########
@@ -153,6 +150,16 @@ public void startEmbedded(@Nullable TestInfo testInfo, int
nodesCount, boolean i
started = true;
}
+ /**
+ * Starts cluster in embedded mode with nodes of current version.
+ *
+ * @param nodesCount Number of nodes in the cluster.
+ * @param initParametersConfigurator the configurator to use for
initializing the cluster.
Review Comment:
Please describe behavior if null is provided
##########
modules/compatibility-tests/src/testFixtures/java/org/apache/ignite/internal/IgniteCluster.java:
##########
@@ -104,32 +104,29 @@ public class IgniteCluster {
*
* @param igniteVersion Ignite version to run the nodes with.
* @param nodesCount Number of nodes in the cluster.
- * @param extraIgniteModuleIds Gradle dependency id notation of the extra
dependencies
- * that should be loaded together with
requested ignite version.
+ * @param extraIgniteModuleIds Gradle dependency id notation of the extra
dependencies that should be loaded together with
+ * requested ignite version.
*/
public void start(String igniteVersion, int nodesCount, List<String>
extraIgniteModuleIds) {
if (started) {
throw new IllegalStateException("The cluster is already started");
}
- runnerNodes = startRunnerNodes(igniteVersion, nodesCount,
extraIgniteModuleIds);
+ startRunnerNodes(igniteVersion, nodesCount, extraIgniteModuleIds);
}
/**
* Starts cluster in embedded mode with nodes of current version.
*
+ * @param testInfo Test info.
* @param nodesCount Number of nodes in the cluster.
+ * @param initParametersConfigurator the configurator to use for
initializing the cluster.
Review Comment:
Please describe behavior if null is provided
##########
modules/compatibility-tests/src/testFixtures/java/org/apache/ignite/internal/IgniteCluster.java:
##########
@@ -173,18 +180,17 @@ public void stop() {
LOG.info("Shut the embedded cluster down");
- if (runnerNodes != null) {
- List<String> nodeNames = runnerNodes.stream()
- .map(RunnerNode::nodeName)
- .collect(toList());
+ List<String> nodeNames = runnerNodes.stream()
+ .filter(Objects::nonNull)
Review Comment:
With CopyOnWriteList you will not need this filter. Also I do not see a way
to have null in that list
##########
modules/compatibility-tests/src/testFixtures/java/org/apache/ignite/internal/IgniteCluster.java:
##########
@@ -104,32 +104,29 @@ public class IgniteCluster {
*
* @param igniteVersion Ignite version to run the nodes with.
* @param nodesCount Number of nodes in the cluster.
- * @param extraIgniteModuleIds Gradle dependency id notation of the extra
dependencies
- * that should be loaded together with
requested ignite version.
+ * @param extraIgniteModuleIds Gradle dependency id notation of the extra
dependencies that should be loaded together with
Review Comment:
Change is not related to PR
##########
modules/compatibility-tests/src/integrationTest/java/org/apache/ignite/internal/client/OldClientWithCurrentServerCompatibilityTest.java:
##########
@@ -69,7 +69,7 @@ void beforeAll(String clientVer, TestInfo testInfo,
@WorkDirectory Path workDir)
clientVersion = clientVer;
cluster = CompatibilityTestBase.createCluster(testInfo, workDir,
CompatibilityTestBase.NODE_BOOTSTRAP_CFG_TEMPLATE);
- cluster.startEmbedded(1, true);
+ cluster.startEmbedded(1, x -> {});
Review Comment:
Why empty lambda here and not null?
##########
modules/compatibility-tests/src/testFixtures/java/org/apache/ignite/internal/IgniteCluster.java:
##########
@@ -334,34 +348,43 @@ private ServerRegistration startEmbeddedNode(
return new ServerRegistration(node, registrationFuture);
}
- private List<RunnerNode> startRunnerNodes(String igniteVersion, int
nodesCount, List<String> extraIgniteModuleIds) {
- try (ProjectConnection connection = GradleConnector.newConnector()
+ private static ProjectConnection getProjectConnection() {
Review Comment:
Let's move all 3 of this methods to the end. It's better to separate utility
static methods and common methods. Also it will be way easier to understand
what is going on during review.
##########
modules/compatibility-tests/src/testFixtures/java/org/apache/ignite/internal/IgniteCluster.java:
##########
@@ -194,7 +200,7 @@ public void stop() {
* Initializes the cluster using REST API on the first node with default
settings.
*/
void init(Consumer<InitParametersBuilder> initParametersConfigurator) {
- init(new int[] { 0 }, initParametersConfigurator);
+ init(new int[]{0}, initParametersConfigurator);
Review Comment:
Change is not related to PR
##########
modules/compatibility-tests/src/testFixtures/java/org/apache/ignite/internal/IgniteCluster.java:
##########
@@ -294,7 +300,15 @@ public List<Ignite> nodes() {
return nodes;
}
- private ServerRegistration startEmbeddedNode(
+ /**
+ * Starts an embedded node with the given index.
+ *
+ * @param testInfo Test info.
+ * @param nodeIndex Index of the node to start.
+ * @param nodesCount the total number of nodes in the cluster.
+ * @return Server registration and future that completes when the node is
fully started and joined the cluster.
+ */
+ public ServerRegistration startEmbeddedNode(
Review Comment:
Doesn't need to be public at all
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
