[STORM-3683] Check if JVM options used for launching worker are valid. #3319

bipinprasad · 2020-08-04T18:55:32Z

What is the purpose of the change

Error in JVM options can cause the worker launch to fail. This change will detect the erroneous options to be detected early when topology is submitted.

How was the change tested

With new unit tests

agresch · 2020-08-04T18:59:27Z

storm-client/src/jvm/org/apache/storm/utils/Utils.java

@@ -1907,4 +1909,201 @@ private void readArchive(ZipFile zipFile) throws IOException {
            }
        }
    }
+
+    /**
+     * Return path to the Java command "x", prefixing with $}{JAVA_HOME}/bin/ if JAVA_HOME system property is defined.


extra } bracket in $}{JAVA_HOME}

agresch · 2020-08-04T18:59:43Z

storm-client/src/jvm/org/apache/storm/utils/Utils.java

+
+    /**
+     * Return path to the Java command "x", prefixing with $}{JAVA_HOME}/bin/ if JAVA_HOME system property is defined.
+     * Otherwie return the supplied Java command unmodified.


Otherwise spelling

agresch · 2020-08-06T16:36:39Z

storm-client/src/jvm/org/apache/storm/utils/Utils.java

+     * @param cmd Java command, e.g. "java", "jar" etc.
+     * @return command string to use.
+     */
+    public static String getJavaCmd(String cmd) {


Thanks for adding this.

agresch · 2020-08-06T16:42:01Z

storm-server/src/main/java/org/apache/storm/daemon/supervisor/BasicContainer.java

@@ -568,14 +570,7 @@ private int getMemOffHeap(WorkerResources resources) {
    }

    protected String javaCmd(String cmd) {


Is there any need to keep this method now that it is available in Utils?

When I took this out, a bunch of test classes started failing. They were using a Mock class to override this class method to return just "java". And in the test, they check the returned array. In order to avoid changing a whole bunch of other classes, I left this signature unchanged. But this definitely looks ugly and the tests should be fixed.

Sounds good. I'm fine keeping this for now.

agresch · 2020-08-06T16:43:35Z

storm-client/src/jvm/org/apache/storm/utils/Utils.java

+    public static boolean validateWorkerLaunchOptions(Map<String, Object> supervisorConf, Map<String, Object> topoConf,
+                                                      Map<String, Object> substitutions, boolean throwExceptionOnFailure)
+            throws InvalidTopologyException {
+        // from storm-server/.../BasicContainer.mkLaunchCommand


Can we commonize this, or grab them from a BasicContainer method? Hard to maintain this.

BasicContainer is in storm-server package which depends on the storm-client package for compile.
Should be commonized somehow - otherwise this is a hidden forward dependency. Will look into this.

Note that storm-server:BasicContainer.java is now using storm-client:Utils.WellKnownRuntimeSubstitutionVars. Is this sufficient?

I'd like others to chime in here. I like the intent of this change, but the way we're replicating the code here so specifically I'm not so happy with. I understand the dependency issue causes problems.

I don't like the replication either. It adds a lot of maintenance overhead. The suggestion I can think of is to move the whole check into server side at Nimbus.submitTopologyWithOpts, which is better in a few ways. But it has its own problem to be taken care of (see my comments above).

Some thoughts on this:
This feature (check GC option conflicts) is nice to have. But it is not hard to find out that workers fail because of GC option conflicts. So I think we don't have to implement this feature if there is no good/clean way to implement it.

bipinprasad · 2020-08-12T14:34:03Z

close/reopen for rebuild

Ethanlm · 2020-09-01T16:10:01Z

storm-client/src/jvm/org/apache/storm/StormSubmitter.java

@@ -528,6 +528,7 @@ private static void validateConfs(Map<String, Object> topoConf, StormTopology to
        InvalidTopologyException, AuthorizationException {
        ConfigValidation.validateTopoConf(topoConf);
        Utils.validateTopologyBlobStoreMap(topoConf);
+        Utils.validateWorkerLaunchOptions(null, topoConf, null, true);


A validation here is not likely to prevent the issue since it doesn't use the supervisorConf here. But in BasicContainer, it will use supervisorConf, which might have some default values.

We can add the check inside Nimbus.submitTopologyWithOpts (server side), and if it fails the check, throws InvalidTopologyException. This can avoid code duplication and avoid exposing too many method on the client side.

But the problem is still it will use nimbusConf instead of supervisorConf, unless we can find a clean way to use the same source of truth.

Ethanlm · 2020-09-01T18:37:32Z

storm-client/src/jvm/org/apache/storm/utils/Utils.java

+    public static boolean validateWorkerLaunchOptions(Map<String, Object> supervisorConf, Map<String, Object> topoConf,
+                                                      Map<String, Object> substitutions, boolean throwExceptionOnFailure)
+            throws InvalidTopologyException {
+        // from storm-server/.../BasicContainer.mkLaunchCommand


I don't like the replication either. It adds a lot of maintenance overhead. The suggestion I can think of is to move the whole check into server side at Nimbus.submitTopologyWithOpts, which is better in a few ways. But it has its own problem to be taken care of (see my comments above).

Some thoughts on this:
This feature (check GC option conflicts) is nice to have. But it is not hard to find out that workers fail because of GC option conflicts. So I think we don't have to implement this feature if there is no good/clean way to implement it.

bipinprasad · 2020-09-03T19:28:06Z

I think a good reason to catching this early (i.e. at submission time) is that the cycle time for error detection is longer. Case in point is JVM options changes and the debug cycle yesterday just to discover the problem with the profiler options.

Ethanlm · 2020-09-04T14:44:57Z

I think a good reason to catching this early (i.e. at submission time) is that the cycle time for error detection is longer.

It is not hard to find out that jvm didn't launch because of jvm option issue, GC option, profiler option or other options, since they are in the logs.

But if this feature is really desired, I would suggest to move to Nimbus code. Doing it in Nimbus.submitTopologyWithOpts is still at submission time and users will see the exception from console log. But doing it on StormSubmitter side won't be able to detect the issue because it doesn't use daemonConf.

Another advantage of doing it at Nimbus.submitTopologyWithOpts is we can avoid code duplication and we can restrict the scope of the code related to launching a worker. Because the code to launch the worker is purely server-side, it would be better to not expose it to client.

But after moving it to Nimbus code, we need to deal with nimbusConf vs supervisorConf issue (although it is a less serious problem)

[STORM-3683] Check if JVM options used for launching worker are valid.

20d3d5d

bipinprasad force-pushed the storm3683 branch from 2ab7aac to 20d3d5d Compare August 4, 2020 20:30

bipinprasad closed this Aug 6, 2020

bipinprasad reopened this Aug 6, 2020

agresch reviewed Aug 6, 2020

View reviewed changes

[STORM-3683] Fix javadoc.

cc06293

bipinprasad closed this Aug 12, 2020

bipinprasad reopened this Aug 12, 2020

Ethanlm reviewed Sep 1, 2020

View reviewed changes

rzo1 added the stale-but-worth-a-look label Aug 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[STORM-3683] Check if JVM options used for launching worker are valid. #3319

[STORM-3683] Check if JVM options used for launching worker are valid. #3319

bipinprasad commented Aug 4, 2020

agresch Aug 4, 2020

bipinprasad Aug 7, 2020

agresch Aug 4, 2020

bipinprasad Aug 7, 2020

agresch Aug 6, 2020

agresch Aug 6, 2020

bipinprasad Aug 6, 2020

agresch Aug 6, 2020

agresch Aug 6, 2020

bipinprasad Aug 6, 2020

bipinprasad Aug 7, 2020

agresch Aug 28, 2020

Ethanlm Sep 1, 2020 •

edited

bipinprasad commented Aug 12, 2020

Ethanlm Sep 1, 2020

Ethanlm Sep 1, 2020 •

edited

bipinprasad commented Sep 3, 2020

Ethanlm commented Sep 4, 2020

		@@ -568,14 +570,7 @@ private int getMemOffHeap(WorkerResources resources) {
		}

		protected String javaCmd(String cmd) {

[STORM-3683] Check if JVM options used for launching worker are valid. #3319

Are you sure you want to change the base?

[STORM-3683] Check if JVM options used for launching worker are valid. #3319

Conversation

bipinprasad commented Aug 4, 2020

What is the purpose of the change

How was the change tested

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ethanlm Sep 1, 2020 • edited

Choose a reason for hiding this comment

bipinprasad commented Aug 12, 2020

Choose a reason for hiding this comment

Ethanlm Sep 1, 2020 • edited

Choose a reason for hiding this comment

bipinprasad commented Sep 3, 2020

Ethanlm commented Sep 4, 2020

Ethanlm Sep 1, 2020 •

edited

Ethanlm Sep 1, 2020 •

edited