azkaban-developers

diff --git a/.gitignore b/.gitignore
index 60d8513..356c17c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,5 +1,5 @@
-build/
 bin/
+build/
 .gradle/
 .settings/
 .idea/

diff --git a/azkaban-common/src/main/java/azkaban/project/ProjectManager.java b/azkaban-common/src/main/java/azkaban/project/ProjectManager.java
index dc24544..64c450d 100644
--- a/azkaban-common/src/main/java/azkaban/project/ProjectManager.java
+++ b/azkaban-common/src/main/java/azkaban/project/ProjectManager.java
@@ -33,7 +33,7 @@ import org.apache.log4j.Logger;
 
 import azkaban.flow.Flow;
 import azkaban.project.ProjectLogEvent.EventType;
-import azkaban.project.validator.Status;
+import azkaban.project.validator.ValidationStatus;
 import azkaban.project.validator.ValidationReport;
 import azkaban.project.validator.ValidatorManager;
 import azkaban.project.validator.XmlValidatorManager;
@@ -47,7 +47,7 @@ import azkaban.utils.Utils;
 public class ProjectManager {
   private static final Logger logger = Logger.getLogger(ProjectManager.class);
 
-  public static final String PROJECT_ARCHIVE_FILE = "project.archive.file";
+  public static final String PROJECT_ARCHIVE_FILE_PATH = "project.archive.file.path";
 
   private ConcurrentHashMap<Integer, Project> projectsById =
       new ConcurrentHashMap<Integer, Project>();
@@ -366,18 +366,18 @@ public class ProjectManager {
       throw new ProjectManagerException("Error unzipping file.", e);
     }
 
-    props.put(PROJECT_ARCHIVE_FILE, archive.getAbsolutePath());
+    props.put(PROJECT_ARCHIVE_FILE_PATH, archive.getAbsolutePath());
     validatorManager.loadValidators(props, logger);
-    logger.info("Validating project using the registered validators "
+    logger.info("Validating project " + archive.getName() + " using the registered validators "
         + validatorManager.getValidatorsInfo().toString());
     Map<String, ValidationReport> reports = validatorManager.validate(file);
-    Status status = Status.PASS;
+    ValidationStatus status = ValidationStatus.PASS;
     for (Entry<String, ValidationReport> report : reports.entrySet()) {
       if (report.getValue().getStatus().compareTo(status) > 0) {
         status = report.getValue().getStatus();
       }
     }
-    if (status == Status.ERROR) {
+    if (status == ValidationStatus.ERROR) {
       logger.error("Error found in upload to " + project.getName()
           + ". Cleaning up.");
 

diff --git a/azkaban-common/src/main/java/azkaban/project/validator/ProjectValidator.java b/azkaban-common/src/main/java/azkaban/project/validator/ProjectValidator.java
index f89e150..4cdd512 100644
--- a/azkaban-common/src/main/java/azkaban/project/validator/ProjectValidator.java
+++ b/azkaban-common/src/main/java/azkaban/project/validator/ProjectValidator.java
@@ -4,8 +4,34 @@ import java.io.File;
 
 import azkaban.utils.Props;
 
+/**
+ * Interface to be implemented by plugins which are to be registered with Azkaban
+ * as project validators that validate a project before uploaded into Azkaban.
+ */
 public interface ProjectValidator {
+
+  /**
+   * Initialize the validator using the given properties.
+   *
+   * @param configuration
+   * @return
+   */
   boolean initialize(Props configuration);
+
+  /**
+   * Return the name of the validator.
+   *
+   * @return
+   */
   String getValidatorInfo();
+
+  /**
+   * Validate the project inside the given directory. The validator, using its own
+   * validation logic, will generate a {@link ValidationReport} representing the result of
+   * the validation.
+   *
+   * @param projectDir
+   * @return
+   */
   ValidationReport validateProject(File projectDir);
 }

diff --git a/azkaban-common/src/main/java/azkaban/project/validator/ValidationReport.java b/azkaban-common/src/main/java/azkaban/project/validator/ValidationReport.java
index 0a87a1e..9cccffb 100644
--- a/azkaban-common/src/main/java/azkaban/project/validator/ValidationReport.java
+++ b/azkaban-common/src/main/java/azkaban/project/validator/ValidationReport.java
@@ -3,50 +3,99 @@ package azkaban.project.validator;
 import java.util.HashSet;
 import java.util.Set;
 
+/**
+ * The result of a project validation generated by a {@link ProjectValidator}. It contains
+ * an enum of type {@link ValidationStatus} representing whether the validation passes,
+ * generates warnings, or generates errors. Accordingly, three sets of String are also
+ * maintained, storing the messages generated by the {@link ProjectValidator} at each of
+ * the 3 {@link ValidationStatus} levels, i.e., {@link ValidationStatus#PASS},
+ * {@link ValidationStatus#WARN}, and {@link ValidationStatus#ERROR}.
+ */
 public class ValidationReport {
 
-  protected Status _status;
+  protected ValidationStatus _status;
   protected Set<String> _passMsgs;
   protected Set<String> _warningMsgs;
   protected Set<String> _errorMsgs;
 
   public ValidationReport() {
-    _status = Status.PASS;
+    _status = ValidationStatus.PASS;
     _passMsgs = new HashSet<String>();
     _warningMsgs = new HashSet<String>();
     _errorMsgs = new HashSet<String>();
   }
 
+  /**
+   * Add a message with status level being {@link ValidationStatus#PASS}
+   *
+   * @param msgs
+   */
   public void addPassMsgs(Set<String> msgs) {
-    _passMsgs.addAll(msgs);
+    if (msgs != null) {
+      _passMsgs.addAll(msgs);
+    }
   }
 
+  /**
+   * Add a message with status level being {@link ValidationStatus#WARN}
+   *
+   * @param msgs
+   */
   public void addWarningMsgs(Set<String> msgs) {
-    _warningMsgs.addAll(msgs);
-    if (!msgs.isEmpty() && _errorMsgs.isEmpty()) {
-      _status = Status.WARN;
+    if (msgs != null) {
+      _warningMsgs.addAll(msgs);
+      if (!msgs.isEmpty() && _errorMsgs.isEmpty()) {
+        _status = ValidationStatus.WARN;
+      }
     }
   }
 
+  /**
+   * Add a message with status level being {@link ValidationStatus#ERROR}
+   *
+   * @param msgs
+   */
   public void addErrorMsgs(Set<String> msgs) {
-    _errorMsgs.addAll(msgs);
-    if (!msgs.isEmpty()) {
-      _status = Status.ERROR;
+    if (msgs != null) {
+      _errorMsgs.addAll(msgs);
+      if (!msgs.isEmpty()) {
+        _status = ValidationStatus.ERROR;
+      }
     }
   }
 
-  public Status getStatus() {
+  /**
+   * Retrieve the status of the report.
+   *
+   * @return
+   */
+  public ValidationStatus getStatus() {
     return _status;
   }
 
+  /**
+   * Retrieve the messages associated with status level {@link ValidationStatus#PASS}
+   *
+   * @return
+   */
   public Set<String> getPassMsgs() {
     return _passMsgs;
   }
 
+  /**
+   * Retrieve the messages associated with status level {@link ValidationStatus#WARN}
+   *
+   * @return
+   */
   public Set<String> getWarningMsgs() {
     return _warningMsgs;
   }
 
+  /**
+   * Retrieve the messages associated with status level {@link ValidationStatus#ERROR}
+   *
+   * @return
+   */
   public Set<String> getErrorMsgs() {
     return _errorMsgs;
   }

diff --git a/azkaban-common/src/main/java/azkaban/project/validator/ValidatorManager.java b/azkaban-common/src/main/java/azkaban/project/validator/ValidatorManager.java
index d1b4ffa..1be62fb 100644
--- a/azkaban-common/src/main/java/azkaban/project/validator/ValidatorManager.java
+++ b/azkaban-common/src/main/java/azkaban/project/validator/ValidatorManager.java
@@ -8,12 +8,44 @@ import org.apache.log4j.Logger;
 
 import azkaban.utils.Props;
 
+/**
+ * ValidatorManager is responsible for loading the list of validators specified in the
+ * Azkaban validator configuration file. Once these validators are loaded, the ValidatorManager
+ * will use the registered validators to verify each uploaded project before persisting it.
+ */
 public interface ValidatorManager {
+  /**
+   * Load the validators using the given properties. Each validator is also given the specified
+   * logger to record any necessary message in the Azkaban log file.
+   *
+   * @param props
+   * @param logger
+   */
   void loadValidators(Props props, Logger logger);
 
+  /**
+   * Validate the given project using the registered list of validators. This method returns a
+   * map of {@link ValidationReport} with the key being the validator's name and the value being
+   * the {@link ValidationReport} generated by that validator.
+   *
+   * @param projectDir
+   * @return
+   */
   Map<String, ValidationReport> validate(File projectDir);
 
+  /**
+   * The ValidatorManager should have a default validator which checks for the most essential
+   * components of a project. The ValidatorManager should always load the default validator.
+   * This method returns the default validator of this ValidatorManager.
+   *
+   * @return
+   */
   ProjectValidator getDefaultValidator();
 
+  /**
+   * Returns a list of String containing the name of each registered validators.
+   *
+   * @return
+   */
   List<String> getValidatorsInfo();
 }

diff --git a/azkaban-common/src/main/java/azkaban/project/validator/XmlValidatorManager.java b/azkaban-common/src/main/java/azkaban/project/validator/XmlValidatorManager.java
index ebd12dc..7b6aa38 100644
--- a/azkaban-common/src/main/java/azkaban/project/validator/XmlValidatorManager.java
+++ b/azkaban-common/src/main/java/azkaban/project/validator/XmlValidatorManager.java
@@ -26,9 +26,21 @@ import org.xml.sax.SAXException;
 import azkaban.utils.DirectoryFlowLoader;
 import azkaban.utils.Props;
 
+/**
+ * Xml implementation of the ValidatorManager. Looks for the property
+ * project.validators.xml.file in the azkaban properties.
+ *
+ * The xml to be in the following form:
+ * <azkaban-validators>
+ *   <validator classname="validator class name">
+ *     <!-- optional configurations for each individual validator -->
+ *     <item key="validator property key" value="validator property value" />
+ *     ...
+ *   </validator>
+ * </azkaban-validators>
+ */
 public class XmlValidatorManager implements ValidatorManager {
-  private static final Logger logger = Logger.getLogger(XmlValidatorManager.class
-      .getName());
+  private static final Logger logger = Logger.getLogger(XmlValidatorManager.class);
 
   public static final String DEFAULT_VALIDATOR_DIR = "validators";
   public static final String VALIDATOR_PLUGIN_DIR = "project.validators.dir";
@@ -42,12 +54,19 @@ public class XmlValidatorManager implements ValidatorManager {
   private Map<String, ProjectValidator> validators;
   private ClassLoader validatorLoader;
 
+  /**
+   * Load the validator plugins from the validator directory (default being validators/) into
+   * the validator ClassLoader. This enables creating instances of these validators in the
+   * loadValidators() method.
+   *
+   * @param props
+   */
   public XmlValidatorManager(Props props) {
     String validatorDirPath = props.getString(VALIDATOR_PLUGIN_DIR, DEFAULT_VALIDATOR_DIR);
     File validatorDir = new File(validatorDirPath);
     if (!validatorDir.canRead() || !validatorDir.isDirectory()) {
       throw new ValidatorManagerException("Validator directory " + validatorDirPath
-          + " does not exist or is not a direcotry.");
+          + " does not exist or is not a directory.");
     }
 
     List<URL> resources = new ArrayList<URL>();
@@ -68,11 +87,20 @@ public class XmlValidatorManager implements ValidatorManager {
     try {
       loadValidators(props, logger);
     } catch (Exception e) {
-      logger.error("Cannot load all the validaotors.");
+      logger.error("Cannot load all the validators.");
       throw new ValidatorManagerException(e);
     }
   }
 
+  /**
+   * Instances of the validators are created here rather than in the constructors. This is because
+   * some validators might need to maintain project-specific states, such as {@link DirectoryFlowLoader}.
+   * By instantiating the validators here, it ensures that the validator objects are project-specific,
+   * rather than global.
+   *
+   * {@inheritDoc}
+   * @see azkaban.project.validator.ValidatorManager#loadValidators(azkaban.utils.Props, org.apache.log4j.Logger)
+   */
   @Override
   public void loadValidators(Props props, Logger log) {
     validators = new LinkedHashMap<String, ProjectValidator>();
@@ -81,11 +109,13 @@ public class XmlValidatorManager implements ValidatorManager {
     validators.put(flowLoader.getValidatorInfo(), flowLoader);
 
     if (!props.containsKey(XML_FILE_PARAM)) {
+      logger.warn("Azkaban properties file does not contain the key " + XML_FILE_PARAM);
       return;
     }
     String xmlPath = props.get(XML_FILE_PARAM);
     File file = new File(xmlPath);
     if (!file.exists()) {
+      logger.error("Azkaban validator configuration file " + xmlPath + " does not exist.");
       return;
     }
 

diff --git a/azkaban-common/src/test/java/azkaban/project/validator/XmlValidatorManagerTest.java b/azkaban-common/src/test/java/azkaban/project/validator/XmlValidatorManagerTest.java
new file mode 100644
index 0000000..fb08411
--- /dev/null
+++ b/azkaban-common/src/test/java/azkaban/project/validator/XmlValidatorManagerTest.java
@@ -0,0 +1,93 @@
+package azkaban.project.validator;
+
+import static org.junit.Assert.*;
+
+import java.net.URL;
+
+import org.junit.Test;
+
+import com.google.common.io.Resources;
+
+import azkaban.utils.Props;
+
+public class XmlValidatorManagerTest {
+  private Props baseProps = new Props();
+
+  /**
+   * Test that if the validator directory does not exist, XmlValidatorManager
+   * should throw an exception.
+   */
+  @Test
+  public void testNoValidatorsDir() {
+    Props props = new Props(baseProps);
+
+    try {
+      @SuppressWarnings("unused")
+      XmlValidatorManager manager = new XmlValidatorManager(props);
+    } catch(ValidatorManagerException e) {
+      return;
+    }
+
+    fail("XmlValidatorManager should throw an exception when the validator directory does not exist.");
+  }
+
+  /**
+   * Test that if the validator directory exists but the xml configuration file does not,
+   * XmlValidatorManager only loads the default validator.
+   */
+  @Test
+  public void testDefaultValidator() {
+    Props props = new Props(baseProps);
+    URL validatorUrl = Resources.getResource("project/testValidators");
+    props.put(XmlValidatorManager.VALIDATOR_PLUGIN_DIR, validatorUrl.getPath());
+
+    XmlValidatorManager manager = new XmlValidatorManager(props);
+    assertEquals("XmlValidatorManager should contain only the default validator when no xml configuration "
+        + "file is present.", manager.getValidatorsInfo().size(), 1);
+    assertEquals("XmlValidatorManager should contain only the default validator when no xml configuration "
+        + "file is present.", manager.getValidatorsInfo().get(0), XmlValidatorManager.DEFAULT_VALIDATOR_KEY);
+  }
+
+  /**
+   * Test that if the xml config file specifies a validator classname that does not exist,
+   * XmlValidatorManager should throw an exception.
+   */
+  @Test
+  public void testValidatorDoesNotExist() {
+    Props props = new Props(baseProps);
+    URL validatorUrl = Resources.getResource("project/testValidators");
+    URL configUrl = Resources.getResource("test-conf/azkaban-validators-test1.xml");
+    props.put(XmlValidatorManager.VALIDATOR_PLUGIN_DIR, validatorUrl.getPath());
+    props.put(XmlValidatorManager.XML_FILE_PARAM,
+        configUrl.getPath());
+
+    try {
+      @SuppressWarnings("unused")
+      XmlValidatorManager manager = new XmlValidatorManager(props);
+    } catch(ValidatorManagerException e) {
+      return;
+    }
+
+    fail("XmlValidatorManager should throw an exception when the validator class cannot be found.");
+  }
+
+  /**
+   * Test that if the xml config file is properly set, XmlValidatorManager loads both the default
+   * validator and the one specified in the xml file.
+   */
+  @Test
+  public void testLoadValidators() {
+    Props props = new Props(baseProps);
+    URL validatorUrl = Resources.getResource("project/testValidators");
+    URL configUrl = Resources.getResource("test-conf/azkaban-validators-test2.xml");
+    props.put(XmlValidatorManager.VALIDATOR_PLUGIN_DIR, validatorUrl.getPath());
+    props.put(XmlValidatorManager.XML_FILE_PARAM,
+        configUrl.getPath());
+
+    XmlValidatorManager manager = new XmlValidatorManager(props);
+    assertEquals("XmlValidatorManager should contain 2 validators.", manager.getValidatorsInfo().size(), 2);
+    assertEquals("XmlValidatorManager should contain the validator specified in the xml configuration file.",
+        manager.getValidatorsInfo().get(1), "Test");
+  }
+
+}

diff --git a/azkaban-common/src/test/resources/project/testValidators/test.jar b/azkaban-common/src/test/resources/project/testValidators/test.jar
new file mode 100644
index 0000000..339ec9e
Binary files /dev/null and b/azkaban-common/src/test/resources/project/testValidators/test.jar differ

diff --git a/azkaban-common/src/test/resources/test-conf/azkaban-validators-test1.xml b/azkaban-common/src/test/resources/test-conf/azkaban-validators-test1.xml
new file mode 100644
index 0000000..2d0d772
--- /dev/null
+++ b/azkaban-common/src/test/resources/test-conf/azkaban-validators-test1.xml
@@ -0,0 +1,3 @@
+<azkaban-validators>
+  <validator classname="do.not.exist.validator" />
+</azkaban-validators>
\ No newline at end of file

diff --git a/azkaban-common/src/test/resources/test-conf/azkaban-validators-test2.xml b/azkaban-common/src/test/resources/test-conf/azkaban-validators-test2.xml
new file mode 100644
index 0000000..f9651a0
--- /dev/null
+++ b/azkaban-common/src/test/resources/test-conf/azkaban-validators-test2.xml
@@ -0,0 +1,3 @@
+<azkaban-validators>
+  <validator classname="azkaban.project.validator.TestValidator" />
+</azkaban-validators>
\ No newline at end of file

diff --git a/azkaban-soloserver/.gitignore b/azkaban-soloserver/.gitignore
index 46699ee..5ab4d17 100644
--- a/azkaban-soloserver/.gitignore
+++ b/azkaban-soloserver/.gitignore
@@ -1,8 +1,8 @@
-data/
 conf/
-temp/
-plugins/
-validators/
+data/
 executions/
+plugins/
 projects/
+temp/
+validators/
 *.log

diff --git a/azkaban-webserver/src/main/java/azkaban/webapp/servlet/ProjectManagerServlet.java b/azkaban-webserver/src/main/java/azkaban/webapp/servlet/ProjectManagerServlet.java
index ac172a9..f8215aa 100644
--- a/azkaban-webserver/src/main/java/azkaban/webapp/servlet/ProjectManagerServlet.java
+++ b/azkaban-webserver/src/main/java/azkaban/webapp/servlet/ProjectManagerServlet.java
@@ -1470,7 +1470,7 @@ public class ProjectManagerServlet extends LoginAbstractAzkabanServlet {
             }
           }
         }
-        if (message.toString().length() > 0) {
+        if (message.length() > 0) {
           ret.put("error", message.toString());
         }
       } catch (Exception e) {