[GitHub] [hadoop] tomicooler commented on a change in pull request #3220: YARN-10355. Refactor NM ContainerLaunch.java#orderEnvByDependencies

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

tomicooler commented on a change in pull request #3220:
URL: https://github.com/apache/hadoop/pull/3220#discussion_r673911895



##########
File path: hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/launcher/ContainerLaunch.java
##########
@@ -1382,73 +1410,53 @@ public void setExitOnFailure() {
         return Collections.emptySet();
       }
       final Set<String> deps = new HashSet<>();
-      // env/whitelistedEnv dump values inside double quotes
-      boolean inDoubleQuotes = true;
-      char c;
-      int i = 0;
-      final int len = envVal.length();
-      while (i < len) {
-        c = envVal.charAt(i);
-        if (c == '"') {
-          inDoubleQuotes = !inDoubleQuotes;
-        } else if (c == '\'' && !inDoubleQuotes) {
-          i++;
-          // eat until closing simple quote
-          while (i < len) {
-            c = envVal.charAt(i);
-            if (c == '\\') {
-              i++;
-            }
-            if (c == '\'') {
-              break;
-            }
-            i++;
+      fillDeps(deps, envVal);
+      return deps;
+    }
+
+    private void fillDeps(final Set<String> deps, final String input) {
+      Matcher matcher = mainPattern.matcher(input);
+      while (matcher.find()) {
+        String variableName = matcher.group(VARIABLE_NAME);
+        if (variableName != null && !variableName.isEmpty()) {
+          deps.add(variableName);
+        }
+
+        String doubleQuoted = matcher.group(DOUBLE_QUOTED);
+        if (doubleQuoted != null) {
+          doubleQuoted =
+              singleQuotePattern.matcher(doubleQuoted).replaceAll("");
+          fillDeps(deps, doubleQuoted);
+        }
+
+        String shellExpansion = matcher.group(SHELL_EXPANSION);
+        if (shellExpansion != null && !shellExpansion.isEmpty()) {
+          if (shellExpansion.charAt(0) == '#') {
+            shellExpansion = shellExpansion.substring(1);
           }
-        } else if (c == '\\') {
-          i++;
-        } else if (c == '$') {
-          i++;
-          if (i >= len) {
-            break;
+          String[] split = shellExpansion.split("[:#%/^,@\\[\\]]", 2);

Review comment:
       I extracted and made a Pattern, so the regex won't be compiled every time. I also changed the regex pattern to split at any special character the variable name can't contain. That is more close to the original code's logic.




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

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



---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org