You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "timeabarna (via GitHub)" <gi...@apache.org> on 2023/06/16 13:18:34 UTC

[GitHub] [nifi] timeabarna opened a new pull request, #7388: NIFI-11705 Append Operating System section in NiFi diagnostic tool

timeabarna opened a new pull request, #7388:
URL: https://github.com/apache/nifi/pull/7388

   # Summary
   
   [NIFI-11705](https://issues.apache.org/jira/browse/NIFI-11705)
   
   The nifi.sh script has a diagnostic flag that provides valuable information related to the flow, a thread dump and a lot of other information.
   
   Append "Operating System / Hardware" section with contains number of cores, physical ram, and disk layout.
   
   # Tracking
   
   Please complete the following tracking steps prior to pull request creation.
   
   ### Issue Tracking
   
   - [ ] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
   
   ### Pull Request Tracking
   
   - [ ] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
   - [ ] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
   
   ### Pull Request Formatting
   
   - [ ] Pull Request based on current revision of the `main` branch
   - [ ] Pull Request refers to a feature branch with one commit containing changes
   
   # Verification
   
   Please indicate the verification steps performed prior to pull request creation.
   
   ### Build
   
   - [ ] Build completed using `mvn clean install -P contrib-check`
     - [ ] JDK 11
     - [ ] JDK 17
   
   ### Licensing
   
   - [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
   - [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
   
   ### Documentation
   
   - [ ] Documentation formatting appears as expected in rendered files
   


-- 
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: issues-unsubscribe@nifi.apache.org

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

[GitHub] [nifi] timeabarna commented on a diff in pull request #7388: NIFI-11705 Append Operating System section in NiFi diagnostic tool

Posted by "timeabarna (via GitHub)" <gi...@apache.org>.
timeabarna commented on code in PR #7388:
URL: https://github.com/apache/nifi/pull/7388#discussion_r1233591858


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/ShellCommandExecutor.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell;
+
+import org.apache.nifi.diagnostics.bootstrap.shell.commands.AbstractShellCommand;
+
+import java.io.IOException;
+import java.util.Collection;
+
+public class ShellCommandExecutor {
+
+    public static Collection<String> execute(final AbstractShellCommand command) {

Review Comment:
   ShellCommandExecutor is called from OperatingSystemDiagnosticTask which is in the tasks package. Removing the public visibility would make in inaccessible for this class.



-- 
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: issues-unsubscribe@nifi.apache.org

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

[GitHub] [nifi] timeabarna commented on pull request #7388: NIFI-11705 Append Operating System section in NiFi diagnostic tool

Posted by "timeabarna (via GitHub)" <gi...@apache.org>.
timeabarna commented on PR #7388:
URL: https://github.com/apache/nifi/pull/7388#issuecomment-1600695081

   Hello @exceptionfactory,
   
   Thank you for your recommendations, I've updated the PR accordingly.


-- 
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: issues-unsubscribe@nifi.apache.org

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

[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7388: NIFI-11705 Append Operating System section in NiFi diagnostic tool

Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7388:
URL: https://github.com/apache/nifi/pull/7388#discussion_r1232253734


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/DiagnosticUtils.java:
##########
@@ -19,9 +19,22 @@
 /**
  */
 public final class DiagnosticUtils {
+    final private static String OperatingSystem = System.getProperty("os.name").toLowerCase();
 
     public static int getUtilization(final double used, final double total) {
         return (int) Math.round((used / total) * 100);
     }
 
+    public static boolean isWindows() {
+        return OperatingSystem.contains("win");
+    }
+
+    public static boolean isMac() {
+        return OperatingSystem.contains("mac");
+    }
+
+    public static boolean isLinuxUnix() {
+        return OperatingSystem.contains("nix") ||OperatingSystem.contains("nux") || OperatingSystem.contains("aix");
+    }
+

Review Comment:
   These methods can be replaced with use of Commons Lang3 [SystemUtils](https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/SystemUtils.html)



##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/commands/AbstractShellCommand.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell.commands;

Review Comment:
   Recommend changing `commands` to `command` to follow general conventions that avoid the use of plurals in package names.
   



##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/results/ShellCommandResult.java:
##########
@@ -0,0 +1,23 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell.results;

Review Comment:
   ```suggestion
   package org.apache.nifi.diagnostics.bootstrap.shell.result;
   ```



##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/commands/GetDiskLayoutCommand.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell.commands;
+
+import org.apache.nifi.diagnostics.bootstrap.shell.results.LineSplittingResult;
+
+import java.util.Arrays;
+import java.util.List;
+
+public class GetDiskLayoutCommand extends AbstractShellCommand {
+    private static final String COMMAND_NAME = "GetDiskLayout";
+    private static final List<String> RESULT_LABELS = Arrays.asList("FileSystem/DeviceId", "Total size", "Used", "Free");
+    private static final String REGEX_FOR_SPLITTING = "\\s+";
+    private static final String[] GET_DISK_LAYOUT_FOR_MAC = new String[] {"/bin/sh", "-c", "df -Ph | sed 1d"};
+    private static final String[] GET_DISK_LAYOUT_FOR_LINUX = new String[] {"/bin/sh", "-c", "df -h --output=source,size,used,avail,target -x tmpfs -x devtmpfs | sed 1d"};
+    private static final String[] GET_DISK_LAYOUT_FOR_WINDOWS = new String[] {"powershell.exe", "Get-CIMInstance Win32_LogicalDisk " +

Review Comment:
   Windows supports the `df` command, is there a reason for using powershell instead?



##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/ShellCommandExecutor.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell;
+
+import org.apache.nifi.diagnostics.bootstrap.shell.commands.AbstractShellCommand;
+
+import java.io.IOException;
+import java.util.Collection;
+
+public class ShellCommandExecutor {
+
+    public static Collection<String> execute(final AbstractShellCommand command) {

Review Comment:
   Given the open-ended nature of this utility class, recommend moving it to the `commands` package and removing the `public` visibility.



##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/commands/AbstractShellCommand.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell.commands;

Review Comment:
   ```suggestion
   package org.apache.nifi.diagnostics.bootstrap.shell.command;
   ```



-- 
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: issues-unsubscribe@nifi.apache.org

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

[GitHub] [nifi] timeabarna commented on a diff in pull request #7388: NIFI-11705 Append Operating System section in NiFi diagnostic tool

Posted by "timeabarna (via GitHub)" <gi...@apache.org>.
timeabarna commented on code in PR #7388:
URL: https://github.com/apache/nifi/pull/7388#discussion_r1233586328


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/commands/GetDiskLayoutCommand.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell.commands;
+
+import org.apache.nifi.diagnostics.bootstrap.shell.results.LineSplittingResult;
+
+import java.util.Arrays;
+import java.util.List;
+
+public class GetDiskLayoutCommand extends AbstractShellCommand {
+    private static final String COMMAND_NAME = "GetDiskLayout";
+    private static final List<String> RESULT_LABELS = Arrays.asList("FileSystem/DeviceId", "Total size", "Used", "Free");
+    private static final String REGEX_FOR_SPLITTING = "\\s+";
+    private static final String[] GET_DISK_LAYOUT_FOR_MAC = new String[] {"/bin/sh", "-c", "df -Ph | sed 1d"};
+    private static final String[] GET_DISK_LAYOUT_FOR_LINUX = new String[] {"/bin/sh", "-c", "df -h --output=source,size,used,avail,target -x tmpfs -x devtmpfs | sed 1d"};
+    private static final String[] GET_DISK_LAYOUT_FOR_WINDOWS = new String[] {"powershell.exe", "Get-CIMInstance Win32_LogicalDisk " +

Review Comment:
   df is not a native command for Windows, you need to install a utility or toolkit for it. For example the Windows 10 Pro where I tested the code does not have df command.



-- 
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: issues-unsubscribe@nifi.apache.org

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

[GitHub] [nifi] timeabarna commented on a diff in pull request #7388: NIFI-11705 Append Operating System section in NiFi diagnostic tool

Posted by "timeabarna (via GitHub)" <gi...@apache.org>.
timeabarna commented on code in PR #7388:
URL: https://github.com/apache/nifi/pull/7388#discussion_r1233591858


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/ShellCommandExecutor.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell;
+
+import org.apache.nifi.diagnostics.bootstrap.shell.commands.AbstractShellCommand;
+
+import java.io.IOException;
+import java.util.Collection;
+
+public class ShellCommandExecutor {
+
+    public static Collection<String> execute(final AbstractShellCommand command) {

Review Comment:
   ShellCommandExecutor is called from OperatingSystemDiagnosticTask which is in the tasks package. Removing the public visibility would make it in inaccessible for this class.



-- 
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: issues-unsubscribe@nifi.apache.org

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

[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7388: NIFI-11705 Append Operating System section in NiFi diagnostic tool

Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7388:
URL: https://github.com/apache/nifi/pull/7388#discussion_r1234108361


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/commands/GetDiskLayoutCommand.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell.commands;
+
+import org.apache.nifi.diagnostics.bootstrap.shell.results.LineSplittingResult;
+
+import java.util.Arrays;
+import java.util.List;
+
+public class GetDiskLayoutCommand extends AbstractShellCommand {
+    private static final String COMMAND_NAME = "GetDiskLayout";
+    private static final List<String> RESULT_LABELS = Arrays.asList("FileSystem/DeviceId", "Total size", "Used", "Free");
+    private static final String REGEX_FOR_SPLITTING = "\\s+";
+    private static final String[] GET_DISK_LAYOUT_FOR_MAC = new String[] {"/bin/sh", "-c", "df -Ph | sed 1d"};
+    private static final String[] GET_DISK_LAYOUT_FOR_LINUX = new String[] {"/bin/sh", "-c", "df -h --output=source,size,used,avail,target -x tmpfs -x devtmpfs | sed 1d"};
+    private static final String[] GET_DISK_LAYOUT_FOR_WINDOWS = new String[] {"powershell.exe", "Get-CIMInstance Win32_LogicalDisk " +

Review Comment:
   Thanks for pointing out that detail, that's good to know.



-- 
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: issues-unsubscribe@nifi.apache.org

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

[GitHub] [nifi] exceptionfactory closed pull request #7388: NIFI-11705 Append Operating System section in NiFi diagnostic tool

Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory closed pull request #7388: NIFI-11705 Append Operating System section in NiFi diagnostic tool
URL: https://github.com/apache/nifi/pull/7388


-- 
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: issues-unsubscribe@nifi.apache.org

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

[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7388: NIFI-11705 Append Operating System section in NiFi diagnostic tool

Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7388:
URL: https://github.com/apache/nifi/pull/7388#discussion_r1236186070


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/ShellCommandExecutor.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell;
+
+import org.apache.nifi.diagnostics.bootstrap.shell.command.AbstractShellCommand;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
+
+public class ShellCommandExecutor {
+    private static final Logger logger = LoggerFactory.getLogger(ShellCommandExecutor.class);
+
+    public static Collection<String> execute(final AbstractShellCommand command) {

Review Comment:
   Instead of having this method as a public utility, recommend moving it into `AbstractShellCommand` and defining a `PlatformShellCommand` interface that returns a collection of strings. That hides the invocation details from callers and provides better encapsulation.



##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/ShellCommandExecutor.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell;
+
+import org.apache.nifi.diagnostics.bootstrap.shell.command.AbstractShellCommand;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
+
+public class ShellCommandExecutor {
+    private static final Logger logger = LoggerFactory.getLogger(ShellCommandExecutor.class);
+
+    public static Collection<String> execute(final AbstractShellCommand command) {
+        final ProcessBuilder processBuilder = new ProcessBuilder();
+        processBuilder.command(command.getCommand());
+        try {
+            final Process process = processBuilder.start();
+            return command.getParser().createResult(process);
+        } catch (IndexOutOfBoundsException e) {

Review Comment:
   This does not seem like a good way to catch unsupported operating systems, recommend re-evaluating the approach to avoid attempting to run a command that will fail on an unknown OS.



##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/command/AbstractShellCommand.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell.command;
+
+import org.apache.commons.lang3.SystemUtils;
+import org.apache.nifi.diagnostics.bootstrap.shell.result.ShellCommandResult;
+
+public abstract class AbstractShellCommand {

Review Comment:
   As mentioned on ShellCommandExecutor, recommend defining an interface named something like `DiagnosticShellCommand` or `PlatformShellCommand` with a signature of `Collection<String> execute()`. This abstract class can implement that interface and provide the shared Process execution methods, which will encapsulate the Process execution details.



##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/result/ShellCommandResult.java:
##########
@@ -0,0 +1,23 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell.result;
+
+import java.util.Collection;
+
+public interface ShellCommandResult {
+    Collection<String> createResult(final Process process);

Review Comment:
   It seems like a better abstraction would be passing `InputStream` instead of `Process`. That would allow implementations to be unit-tested with examples.



##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/result/ShellCommandResult.java:
##########
@@ -0,0 +1,23 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell.result;
+
+import java.util.Collection;
+
+public interface ShellCommandResult {
+    Collection<String> createResult(final Process process);

Review Comment:
   ```suggestion
       Collection<String> createResult(final InputStream inputStream);
   ```



##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/result/LineSplittingResult.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell.result;
+
+import java.io.BufferedReader;
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.InputStreamReader;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+public class LineSplittingResult implements ShellCommandResult {
+    private final String regexStringForSplitting;
+    private final List<String> labels;
+    private final String commandName;
+
+    public LineSplittingResult(final String regexStringForSplitting, final List<String> labels, final String commandName) {

Review Comment:
   The `regexStringForSplitting` argument should be changed to use the `Pattern` class, which precompiles the regular expression pattern.



##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/ShellCommandExecutor.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell;
+
+import org.apache.nifi.diagnostics.bootstrap.shell.command.AbstractShellCommand;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
+
+public class ShellCommandExecutor {
+    private static final Logger logger = LoggerFactory.getLogger(ShellCommandExecutor.class);
+
+    public static Collection<String> execute(final AbstractShellCommand command) {
+        final ProcessBuilder processBuilder = new ProcessBuilder();
+        processBuilder.command(command.getCommand());
+        try {
+            final Process process = processBuilder.start();
+            return command.getParser().createResult(process);
+        } catch (IndexOutOfBoundsException e) {
+            logger.warn(String.format("Operating system is not supported, failed to execute command: %s, ", command.getName()));
+            return Collections.EMPTY_LIST;
+        } catch (IOException | NullPointerException e) {

Review Comment:
   It seems like this should be changed to a general `Exception` to cover any other unexpected issues.



##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/command/AbstractShellCommand.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell.command;
+
+import org.apache.commons.lang3.SystemUtils;
+import org.apache.nifi.diagnostics.bootstrap.shell.result.ShellCommandResult;
+
+public abstract class AbstractShellCommand {
+    private final String name;
+    private final String[] windowsCommand;
+    private final String[] linuxCommand;
+    private final String[] macCommand;
+    private final ShellCommandResult parser;
+
+    public AbstractShellCommand(final String name, final String[] windowsCommand, final String[] linuxCommand, final String[] macCommand, final ShellCommandResult parser) {
+        this.name = name;
+        this.windowsCommand = windowsCommand;
+        this.linuxCommand = linuxCommand;
+        this.macCommand = macCommand;
+        this.parser = parser;
+    }
+
+    public String getName() {
+        return name;
+    }
+
+    public String[] getCommand() {
+        if (SystemUtils.IS_OS_MAC) {
+            return macCommand;
+        } else if (SystemUtils.IS_OS_UNIX || SystemUtils.IS_OS_LINUX) {
+            return linuxCommand;
+        } else if (SystemUtils.IS_OS_WINDOWS) {
+            return windowsCommand;
+        } else {
+            return new String[] {};
+        }
+    }
+
+    public ShellCommandResult getParser() {

Review Comment:
   This method name does not align with the returned object, is it necessary for the method to be public?



##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/tasks/OperatingSystemDiagnosticTask.java:
##########
@@ -69,11 +74,26 @@ public DiagnosticsDumpElement captureDump(final boolean verbose) {
             }
 
             attributes.forEach((key, value) -> details.add(key + " : " + value));
+            details.addAll(getPhysicalCPUCores());
+            details.addAll(getTotalPhysicalRam());
+            details.addAll(getDiskLayout());
         } catch (final Exception e) {
             logger.error("Failed to obtain Operating System details", e);
             return new StandardDiagnosticsDumpElement("Operating System / Hardware", Collections.singletonList("Failed to obtain Operating System details"));
         }
 
         return new StandardDiagnosticsDumpElement("Operating System / Hardware", details);
     }
+
+    private Collection<String> getPhysicalCPUCores() {
+        return ShellCommandExecutor.execute(new GetPhysicalCpuCoresCommand());

Review Comment:
   After refactoring, this should be changed to create the command instance and then call `execute()` instead of calling `ShellCommandExecutor.execute()`.



-- 
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: issues-unsubscribe@nifi.apache.org

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

[GitHub] [nifi] exceptionfactory commented on a diff in pull request #7388: NIFI-11705 Append Operating System section in NiFi diagnostic tool

Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on code in PR #7388:
URL: https://github.com/apache/nifi/pull/7388#discussion_r1234107619


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/ShellCommandExecutor.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell;
+
+import org.apache.nifi.diagnostics.bootstrap.shell.commands.AbstractShellCommand;
+
+import java.io.IOException;
+import java.util.Collection;
+
+public class ShellCommandExecutor {
+
+    public static Collection<String> execute(final AbstractShellCommand command) {

Review Comment:
   Thanks, moving the method sounds like a good approach.



-- 
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: issues-unsubscribe@nifi.apache.org

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

[GitHub] [nifi] timeabarna commented on a diff in pull request #7388: NIFI-11705 Append Operating System section in NiFi diagnostic tool

Posted by "timeabarna (via GitHub)" <gi...@apache.org>.
timeabarna commented on code in PR #7388:
URL: https://github.com/apache/nifi/pull/7388#discussion_r1233586328


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/commands/GetDiskLayoutCommand.java:
##########
@@ -0,0 +1,42 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell.commands;
+
+import org.apache.nifi.diagnostics.bootstrap.shell.results.LineSplittingResult;
+
+import java.util.Arrays;
+import java.util.List;
+
+public class GetDiskLayoutCommand extends AbstractShellCommand {
+    private static final String COMMAND_NAME = "GetDiskLayout";
+    private static final List<String> RESULT_LABELS = Arrays.asList("FileSystem/DeviceId", "Total size", "Used", "Free");
+    private static final String REGEX_FOR_SPLITTING = "\\s+";
+    private static final String[] GET_DISK_LAYOUT_FOR_MAC = new String[] {"/bin/sh", "-c", "df -Ph | sed 1d"};
+    private static final String[] GET_DISK_LAYOUT_FOR_LINUX = new String[] {"/bin/sh", "-c", "df -h --output=source,size,used,avail,target -x tmpfs -x devtmpfs | sed 1d"};
+    private static final String[] GET_DISK_LAYOUT_FOR_WINDOWS = new String[] {"powershell.exe", "Get-CIMInstance Win32_LogicalDisk " +

Review Comment:
   df is not a native command for Windows, you need to install a utility or toolkit for it. The Windows 10 Pro where I tested the code does not have df command.



-- 
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: issues-unsubscribe@nifi.apache.org

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

[GitHub] [nifi] timeabarna commented on pull request #7388: NIFI-11705 Append Operating System section in NiFi diagnostic tool

Posted by "timeabarna (via GitHub)" <gi...@apache.org>.
timeabarna commented on PR #7388:
URL: https://github.com/apache/nifi/pull/7388#issuecomment-1596591326

   Hello @exceptionfactory,
   
   Thanks for your review. I've updated the PR based on your recommendations.


-- 
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: issues-unsubscribe@nifi.apache.org

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

[GitHub] [nifi] timeabarna commented on a diff in pull request #7388: NIFI-11705 Append Operating System section in NiFi diagnostic tool

Posted by "timeabarna (via GitHub)" <gi...@apache.org>.
timeabarna commented on code in PR #7388:
URL: https://github.com/apache/nifi/pull/7388#discussion_r1233591858


##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/diagnostics/bootstrap/shell/ShellCommandExecutor.java:
##########
@@ -0,0 +1,36 @@
+/*
+ * 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.nifi.diagnostics.bootstrap.shell;
+
+import org.apache.nifi.diagnostics.bootstrap.shell.commands.AbstractShellCommand;
+
+import java.io.IOException;
+import java.util.Collection;
+
+public class ShellCommandExecutor {
+
+    public static Collection<String> execute(final AbstractShellCommand command) {

Review Comment:
   ShellCommandExecutor is called from OperatingSystemDiagnosticTask which is in the tasks package. Removing the public visibility would make it in inaccessible for this class. Would you like to add this method to AbstractShellCommand, remove this class and call Command.execute instead?



-- 
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: issues-unsubscribe@nifi.apache.org

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