[GitHub] [storm] Ethanlm commented on a change in pull request #3319: [STORM-3683] Check if JVM options used for launching worker are valid.

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

Ethanlm commented on a change in pull request #3319:
URL: https://github.com/apache/storm/pull/3319#discussion_r481351414



##########
File path: 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.
+     * Otherwie return the supplied Java command unmodified.
+     *
+     * @param cmd Java command, e.g. "java", "jar" etc.
+     * @return command string to use.
+     */
+    public static String getJavaCmd(String cmd) {
+
+        String ret = null;
+        String javaHome = System.getenv().get("JAVA_HOME");
+        if (StringUtils.isNotBlank(javaHome)) {
+            ret = javaHome + File.separator + "bin" + File.separator + cmd;
+        } else {
+            ret = cmd;
+        }
+        return ret;
+    }
+
+    /**
+     * Find the list of substitutable variable names in supplied string. Substitutable variable names are
+     * preceded and followed by a percent sign and composed of upper case characters, numbers, underscore and dash
+     * characters.
+     *
+     * @param str string with 0 or more substitutable variables.
+     * @return a set with 0 or more variable names.
+     */
+    public static Set<String> findSubstitutableVarNames(String str) {
+        final String patternStr = "%([A-Z0-9_-]+)%";
+        final int matchedGroup = 1; // matched group is 1 because of the capturing parens in the patternStr
+
+        Set<String> ret = new TreeSet<>();
+        Pattern p = Pattern.compile(patternStr);
+        Matcher m = p.matcher(str);
+        int matchCnt = 0;
+        while (m.find()) {
+            matchCnt++;
+            try {
+                ret.add(str.substring(m.start(matchedGroup), m.end(matchedGroup)));
+            } catch (IllegalStateException ex) {
+                String err = String.format("Internal Error in findSubstitutableVarNames(\"%s\"), pattern \"%s\", matchCnt=%d",
+                        str, patternStr, matchCnt, str);
+                LOG.error(err, ex);
+            }
+        }
+        return ret;
+    }
+
+    /**
+     * Perform variable substitutions using the supplied substitutions.
+     * Leave unchanged any placeholders for undefined variables.
+     *
+     * @param str original string with placeholders for replacements.
+     * @param substitutions map of substitution variables and values.
+     * @return final string with variable replaced with supplied substitutions.
+     */
+    public static String substituteVarNames(String str, Map<String, Object> substitutions) {
+        Set<String> vars = findSubstitutableVarNames(str);
+        for (String var : vars) {
+            if (!substitutions.containsKey(var)) {
+                continue;
+            }
+            String orig = "%" + var + "%";
+            String substitution = String.valueOf(substitutions.get(var));
+            str = str.replace(orig, substitution);
+        }
+        return str;
+    }
+
+    public static List<String> substituteVarNamesInObject(Object value, Map<String, Object> substitutions) {
+        List<String> rets = new ArrayList<>();
+        if (value instanceof String) {
+            String string = substituteVarNames((String) value, substitutions);
+            if (StringUtils.isNotBlank(string)) {
+                rets.add(string);
+            }
+        } else if (value instanceof List) {
+            ((List<String>) value).forEach(x -> {
+                x = substituteVarNames(x, substitutions);
+                if (StringUtils.isNotBlank(x)) {
+                    rets.add(x);
+                }
+            });
+        }
+        return rets;
+    }
+
+    /**
+     * Enumeration of variables that can be substituted in Java command string.
+     */
+    public enum WellKnownRuntimeSubstitutionVars {
+        ID("ID"),
+        WORKER_ID("WORKER-ID"),
+        TOPOLOGY_ID("TOPOLOGY-ID"),
+        WORKER_PORT("WORKER-PORT"),
+        HEAP_MEM("HEAP-MEM"),
+        OFF_HEAP_MEM("OFF-HEAP-MEM"),
+        LIMIT_MEM("LIMIT-MEM");
+
+        private String varName;
+
+        WellKnownRuntimeSubstitutionVars(String varName) {
+            this.varName = varName;
+        }
+
+        public String getVarName() {
+            return varName;
+        }
+
+        public static Map<String, Object> getDummySubstitutions() {
+            Map<String, Object> ret = new HashMap<>();
+            ret.put(ID.getVarName(), "dummyId");
+            ret.put(WORKER_ID.getVarName(), "dummy-worker-id");
+            ret.put(TOPOLOGY_ID.getVarName(), "dummy-topology-id");
+            ret.put(WORKER_PORT.getVarName(), 6700);
+            ret.put(HEAP_MEM.getVarName(), 1024);
+            ret.put(OFF_HEAP_MEM.getVarName(), 1024);
+            ret.put(LIMIT_MEM.getVarName(), 1024);
+            return ret;
+        }
+    }
+
+    /**
+     * Launch a validation java command (java -showversion java.util.prefs.Base64 1 1) with JVM options
+     * used in worker launch to validate JVM options.
+     *
+     * @param supervisorConf configuration for the supervisor. May be null.
+     * @param topoConf configuration for the topology. Must be provided.
+     * @param substitutions may be null in which case it is {@link WellKnownRuntimeSubstitutionVars#getDummySubstitutions()}
+     * @param throwExceptionOnFailure if true then an exception is thrown instead of returning false
+     * @return true if the option combination is valid, false otherwise (or throws InvalidTopologyException)
+     */
+    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

Review comment:
       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`. But it has its own problem to be taken care of (see my comments above).
   
   A few 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.

##########
File path: 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);

Review comment:
       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.
   
   
   




----------------------------------------------------------------
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:
users@infra.apache.org