[GitHub] [ignite-3] kgusakov commented on a diff in pull request #905: IGNITE-17091: Implement status command

[GitBox <gi...@apache.org>]

kgusakov commented on code in PR #905:
URL: https://github.com/apache/ignite-3/pull/905#discussion_r919491639


##########
modules/cli/src/integrationTest/java/org/apache/ignite/rest/ItGeneratedRestClientTest.java:
##########
@@ -81,7 +87,10 @@ public class ItGeneratedRestClientTest {
 
     private ClusterManagementApi clusterManagementApi;
 
+    private NodeManagementApi nodeManagementApi;
+
     private ObjectMapper objectMapper;
+    private String firstNodeName;

Review Comment:
   ```suggestion
   
       private String firstNodeName;
   ```



##########
modules/cli/src/main/java/org/apache/ignite/cli/call/cluster/status/ClusterStatusCall.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.cli.call.cluster.status;
+
+import jakarta.inject.Singleton;
+import org.apache.ignite.cli.call.cluster.status.ClusterStatus.ClusterStatusBuilder;
+import org.apache.ignite.cli.core.call.Call;
+import org.apache.ignite.cli.core.call.CallOutput;
+import org.apache.ignite.cli.core.call.DefaultCallOutput;
+import org.apache.ignite.cli.core.call.StatusCallInput;
+import org.apache.ignite.cli.core.exception.IgniteCliApiException;
+import org.apache.ignite.cli.deprecated.CliPathsConfigLoader;
+import org.apache.ignite.cli.deprecated.IgnitePaths;
+import org.apache.ignite.cli.deprecated.builtins.node.NodeManager;
+import org.apache.ignite.rest.client.api.ClusterManagementApi;
+import org.apache.ignite.rest.client.invoker.ApiClient;
+import org.apache.ignite.rest.client.invoker.ApiException;
+import org.apache.ignite.rest.client.model.ClusterState;
+
+/**
+ * Call to get cluster status.
+ */
+@Singleton
+public class ClusterStatusCall implements Call<StatusCallInput, ClusterStatus> {
+
+    private final NodeManager nodeManager;
+
+    private final CliPathsConfigLoader cliPathsCfgLdr;
+
+    /**
+     * Default constructor.
+     */
+    public ClusterStatusCall(NodeManager nodeManager, CliPathsConfigLoader cliPathsCfgLdr) {
+        this.nodeManager = nodeManager;
+        this.cliPathsCfgLdr = cliPathsCfgLdr;
+    }
+
+    @Override
+    public CallOutput<ClusterStatus> execute(StatusCallInput input) {
+        IgnitePaths paths = cliPathsCfgLdr.loadIgnitePathsOrThrowError();
+        ClusterStatusBuilder clusterStatusBuilder = ClusterStatus.builder()
+                .nodeCount(nodeManager.getRunningNodes(paths.logDir, paths.cliPidsDir()).size());
+
+        try {
+            ClusterState clusterState = fetchClusterState(input.getClusterUrl());
+            clusterStatusBuilder
+                    .initialized(true)
+                    .name(clusterState.getClusterTag().getClusterName())
+                    .metadataStorageNodes(clusterState.getMsNodes())
+                    .cmgNodes(clusterState.getCmgNodes());
+        } catch (ApiException e) {
+            if (e.getCode() == 404) { // NOT_FOUND means the cluster is not initialized yet
+                clusterStatusBuilder.initialized(false);
+            } else {
+                return DefaultCallOutput.failure(new IgniteCliApiException(e, input.getClusterUrl()));
+            }
+        } catch (IllegalArgumentException e) {
+            clusterStatusBuilder.initialized(false);

Review Comment:
   Not sure, that I understand, why IllegalArgumentException means, that cluster is not initialized? Could you write small comment?



##########
modules/node-management/pom.xml:
##########
@@ -0,0 +1,156 @@
+<?xml version="1.0" encoding="UTF-8"?>

Review Comment:
   It looks like it is not 'node-management' module, but just module with rest controller and some interfaces for it. Actual node management is doing under 'runner' module. 
   If we create the separate top-level module for this kind of job - I fear we will be drowned in one-controller modules and become the REST factory instead of database :)
   
   I agree, that maybe the runner is not the best place for it, but at the moment it is the real place where node lifecycle manages.
   
   Maybe, if we have other controllers (which can't be placed in any existent modules), we'll should return to this topic and maybe create the separate place or package inside current 'rest*' modules. WDYT?



-- 
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: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org