YARN-11175. Refactor LogAggregationFileControllerFactory

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-common/src/main/java/org/apache/hadoop/yarn/logaggregation/filecontroller/LogAggregationFileControllerFactory.java

@@ -53,9 +53,8 @@ public class LogAggregationFileControllerFactory {
       LogAggregationFileControllerFactory.class);
   private final Pattern p = Pattern.compile(
       "^[A-Za-z_]+[A-Za-z0-9_]*$");
-  private LinkedList<LogAggregationFileController> controllers
+  private final LinkedList<LogAggregationFileController> controllers = new LinkedList<>();
-      = new LinkedList<>();
+  private final Configuration conf;
-  private Configuration conf;
 
   /**
    * Construct the LogAggregationFileControllerFactory object.
@@ -65,77 +64,59 @@ public LogAggregationFileControllerFactory(Configuration conf) {
     this.conf = conf;
     Collection<String> fileControllers = conf.getStringCollection(
         YarnConfiguration.LOG_AGGREGATION_FILE_FORMATS);
-    List<String> controllerClassName = new ArrayList<>();
-
     Map<String, String> controllerChecker = new HashMap<>();
 
-    for (String fileController : fileControllers) {
+    for (String controllerName : fileControllers) {
-      Preconditions.checkArgument(validateAggregatedFileControllerName(
+      validateAggregatedFileControllerName(controllerName);
-          fileController), "The FileControllerName: " + fileController
-          + " set in " + YarnConfiguration.LOG_AGGREGATION_FILE_FORMATS
-          +" is invalid." + "The valid File Controller name should only "
-          + "contain a-zA-Z0-9_ and can not start with numbers");
 
-      String remoteDirStr = String.format(
+      validateConflictingControllers(conf, controllerChecker, controllerName);
-          YarnConfiguration.LOG_AGGREGATION_REMOTE_APP_LOG_DIR_FMT,
+      DeterminedControllerClassName className =
-          fileController);
+          new DeterminedControllerClassName(conf, controllerName);
-      String remoteDir = conf.get(remoteDirStr);
+      LogAggregationFileController controller = createFileControllerInstance(conf,
-      boolean defaultRemoteDir = false;
+          controllerName, className);
-      if (remoteDir == null || remoteDir.isEmpty()) {
+      controller.initialize(conf, controllerName);
-        remoteDir = conf.get(YarnConfiguration.NM_REMOTE_APP_LOG_DIR,
+      controllers.add(controller);
-            YarnConfiguration.DEFAULT_NM_REMOTE_APP_LOG_DIR);
+    }
-        defaultRemoteDir = true;
+  }
-      }
+
-      String suffixStr = String.format(
+  private LogAggregationFileController createFileControllerInstance(
-          YarnConfiguration.LOG_AGGREGATION_REMOTE_APP_LOG_DIR_SUFFIX_FMT,
+      Configuration conf,
-          fileController);
+      String fileController, DeterminedControllerClassName className) {
-      String suffix = conf.get(suffixStr);
+    Class<? extends LogAggregationFileController> clazz = conf.getClass(
-      boolean defaultSuffix = false;
+        className.configKey, null, LogAggregationFileController.class);
-      if (suffix == null || suffix.isEmpty()) {
+    if (clazz == null) {
-        suffix = conf.get(YarnConfiguration.NM_REMOTE_APP_LOG_DIR_SUFFIX,
+      throw new RuntimeException("No class defined for " + fileController);
-            YarnConfiguration.DEFAULT_NM_REMOTE_APP_LOG_DIR_SUFFIX);
+    }
-        defaultSuffix = true;
+    LogAggregationFileController instance = ReflectionUtils.newInstance(clazz, conf);
-      }
+    if (instance == null) {
-      String dirSuffix = remoteDir + "-" + suffix;
+      throw new RuntimeException("No object created for " + className.value);
-      if (controllerChecker.containsKey(dirSuffix)) {
+    }
-        if (defaultRemoteDir && defaultSuffix) {
+    return instance;
-          String fileControllerStr = controllerChecker.get(dirSuffix);
+  }
-          List<String> controllersList = new ArrayList<>();
+
-          controllersList.add(fileControllerStr);
+  private void validateConflictingControllers(
-          controllersList.add(fileController);
+      Configuration conf, Map<String, String> controllerChecker, String fileController) {
-          fileControllerStr = StringUtils.join(controllersList, ",");
+    DeterminedLogAggregationRemoteDir remoteDir =
-          controllerChecker.put(dirSuffix, fileControllerStr);
+        new DeterminedLogAggregationRemoteDir(conf, fileController);
-        } else {
+    DeterminedLogAggregationSuffix suffix =
-          String conflictController = controllerChecker.get(dirSuffix);
+        new DeterminedLogAggregationSuffix(conf, fileController);
-          throw new RuntimeException("The combined value of " + remoteDirStr
+    String dirSuffix = remoteDir.value + "-" + suffix.value;
-              + " and " + suffixStr + " should not be the same as the value"
+    if (controllerChecker.containsKey(dirSuffix)) {
-              + " set for " + conflictController);
+      if (remoteDir.usingDefault && suffix.usingDefault) {
-        }
+        String fileControllerStr = controllerChecker.get(dirSuffix);
+        List<String> controllersList = new ArrayList<>();
+        controllersList.add(fileControllerStr);
+        controllersList.add(fileController);
+        fileControllerStr = StringUtils.join(controllersList, ",");
+        controllerChecker.put(dirSuffix, fileControllerStr);
       } else {
-        controllerChecker.put(dirSuffix, fileController);
+        String conflictController = controllerChecker.get(dirSuffix);
+        throw new RuntimeException(String.format("The combined value of %s " +
+            "and %s should not be the same as the value set for %s",
+            remoteDir.configKey, suffix.configKey, conflictController));
       }
-      String classKey = String.format(
+    } else {
-          YarnConfiguration.LOG_AGGREGATION_FILE_CONTROLLER_FMT,
+      controllerChecker.put(dirSuffix, fileController);
-          fileController);
-      String className = conf.get(classKey);
-      if (className == null || className.isEmpty()) {
-        throw new RuntimeException("No class configured for "
-            + fileController);
-      }
-      controllerClassName.add(className);
-      Class<? extends LogAggregationFileController> sClass = conf.getClass(
-          classKey, null, LogAggregationFileController.class);
-      if (sClass == null) {
-        throw new RuntimeException("No class defined for " + fileController);
-      }
-      LogAggregationFileController s = ReflectionUtils.newInstance(
-          sClass, conf);
-      if (s == null) {
-        throw new RuntimeException("No object created for "
-            + controllerClassName);
-      }
-      s.initialize(conf, fileController);
-      controllers.add(s);
     }
   }
 
@@ -169,8 +150,7 @@ public LogAggregationFileController getFileControllerForRead(
             return fileController;
           }
         } catch (Exception ex) {
-          diagnosticsMsg.append(ex.getMessage() + "\n");
+          diagnosticsMsg.append(ex.getMessage()).append("\n");
-          continue;
         }
       }
     }
@@ -184,19 +164,26 @@ public LogAggregationFileController getFileControllerForRead(
           return fileController;
         }
       } catch (Exception ex) {
-        diagnosticsMsg.append(ex.getMessage() + "\n");
+        diagnosticsMsg.append(ex.getMessage()).append("\n");
-        continue;
       }
     }
 
     throw new IOException(diagnosticsMsg.toString());
   }
 
-  private boolean validateAggregatedFileControllerName(String name) {
+  private void validateAggregatedFileControllerName(String name) {
+    boolean valid;
     if (name == null || name.trim().isEmpty()) {
-      return false;
+      valid = false;
+    } else {
+      valid = p.matcher(name).matches();
     }
-    return p.matcher(name).matches();
+
+    Preconditions.checkArgument(valid,
+            String.format("The FileControllerName: %s set in " +
+                            "%s is invalid.The valid File Controller name should only contain " +
+                            "a-zA-Z0-9_ and cannot start with numbers", name,
+                    YarnConfiguration.LOG_AGGREGATION_FILE_FORMATS));
   }
 
   @Private
@@ -205,4 +192,64 @@ private boolean validateAggregatedFileControllerName(String name) {
       getConfiguredLogAggregationFileControllerList() {
     return this.controllers;
   }
+
+  private static class DeterminedLogAggregationRemoteDir {
+    private String value;
+    private boolean usingDefault = false;
+    private final String configKey;
+
+    DeterminedLogAggregationRemoteDir(Configuration conf,
+        String fileController) {
+      configKey = String.format(
+          YarnConfiguration.LOG_AGGREGATION_REMOTE_APP_LOG_DIR_FMT,
+          fileController);
+      String remoteDir = conf.get(configKey);
+
+      if (remoteDir == null || remoteDir.isEmpty()) {
+        this.value = conf.get(YarnConfiguration.NM_REMOTE_APP_LOG_DIR,
+            YarnConfiguration.DEFAULT_NM_REMOTE_APP_LOG_DIR);
+        this.usingDefault = true;
+      } else {
+        this.value = remoteDir;
+      }
+    }
+  }
+
+  private static class DeterminedLogAggregationSuffix {
+    private String value;
+    private boolean usingDefault = false;
+    private final String configKey;
+
+    DeterminedLogAggregationSuffix(Configuration conf,
+        String fileController) {
+      configKey = String.format(
+          YarnConfiguration.LOG_AGGREGATION_REMOTE_APP_LOG_DIR_SUFFIX_FMT,
+          fileController);
+      String suffix = conf.get(configKey);
+      if (suffix == null || suffix.isEmpty()) {
+        this.value = conf.get(YarnConfiguration.NM_REMOTE_APP_LOG_DIR_SUFFIX,
+            YarnConfiguration.DEFAULT_NM_REMOTE_APP_LOG_DIR_SUFFIX);
+        this.usingDefault = true;
+      } else {
+        this.value = suffix;
+      }
+    }
+  }
+
+  private static class DeterminedControllerClassName {
+    private final String configKey;
+    private final String value;
+
+    DeterminedControllerClassName(Configuration conf,
+        String fileController) {
+      this.configKey = String.format(
+          YarnConfiguration.LOG_AGGREGATION_FILE_CONTROLLER_FMT,
+          fileController);
+      this.value = conf.get(configKey);
+      if (value == null || value.isEmpty()) {
+        throw new RuntimeException("No class configured for "
+            + fileController);
+      }
+    }
+  }
 }