----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33108/#review80482 -----------------------------------------------------------
Overall looks good. I made a number of minor comments. client/src/main/java/org/apache/oozie/cli/OozieCLI.java <https://reviews.apache.org/r/33108/#comment130391> Can you update the description that it handles coordinators and bundles too? Also update the CLI docs where we have a copy-paste of the CLI help message. client/src/main/java/org/apache/oozie/cli/OozieCLI.java <https://reviews.apache.org/r/33108/#comment130392> Can you make this a "TODO" comment? Also create a a JIRA for this. client/src/main/java/org/apache/oozie/cli/OozieCLI.java <https://reviews.apache.org/r/33108/#comment130400> Given that we're keeping this for compatibility, I think we should keep the Unit Test for it. client/src/main/java/org/apache/oozie/client/OozieClient.java <https://reviews.apache.org/r/33108/#comment130394> I think this should just be called "ValidateXML". It's a little confusing when it's called "GetValidateXML", but is doing a POST; not a GET client/src/main/java/org/apache/oozie/client/OozieClient.java <https://reviews.apache.org/r/33108/#comment130396> I think we should make this if (file.startsWith("/") || file.startsWith("file://")) Otherwise, what happens when someone tries S3 or some other FileSystem? client/src/main/java/org/apache/oozie/client/OozieClient.java <https://reviews.apache.org/r/33108/#comment130398> Same as my other comment client/src/main/java/org/apache/oozie/client/OozieClient.java <https://reviews.apache.org/r/33108/#comment130399> You should use isFile() here. If the file is a directory exists() will pass but later on we'll get an error. core/src/main/java/org/apache/oozie/servlet/V2ValidateServlet.java <https://reviews.apache.org/r/33108/#comment130402> We already have a method that handles this in IOUtils called copyCharStream. Can we use that instead? core/src/main/java/org/apache/oozie/servlet/V2ValidateServlet.java <https://reviews.apache.org/r/33108/#comment130403> What does this do? Can you add a comment explaining this? docs/src/site/twiki/DG_CommandLineTool.twiki <https://reviews.apache.org/r/33108/#comment130412> Please also update the REST API docs page (WebServicesAPI.twiki) with the validate command now that it can be used over REST. docs/src/site/twiki/DG_CommandLineTool.twiki <https://reviews.apache.org/r/33108/#comment130408> I would make two separate examples and show both outputs: one with a local file and one with an HDFS file webapp/src/main/webapp/WEB-INF/web.xml <https://reviews.apache.org/r/33108/#comment130409> Please update distro/src/main/tomcat/ssl-web.xml with the validate servlet and servlet mapping too. Otherwise, this won't work when HTTPS is turned on. - Robert Kanter On April 15, 2015, 3:13 p.m., Azrael Park wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33108/ > ----------------------------------------------------------- > > (Updated April 15, 2015, 3:13 p.m.) > > > Review request for oozie. > > > Bugs: OOZIE-2159 > https://issues.apache.org/jira/browse/OOZIE-2159 > > > Repository: oozie-git > > > Description > ------- > > Move 'oozie validate' to server-side. > It can validate both local file and hdfs file. > > > Diffs > ----- > > client/src/main/java/org/apache/oozie/cli/OozieCLI.java aa7ad6c > client/src/main/java/org/apache/oozie/client/OozieClient.java db21e9e > client/src/main/java/org/apache/oozie/client/rest/JsonTags.java ab6699f > client/src/main/java/org/apache/oozie/client/rest/RestConstants.java > 2434b19 > client/src/test/java/org/apache/oozie/cli/TestValidation.java 76c1445 > core/src/main/java/org/apache/oozie/servlet/V2ValidateServlet.java > PRE-CREATION > core/src/test/java/org/apache/oozie/client/TestOozieCLI.java e56b679 > core/src/test/java/org/apache/oozie/servlet/TestV2ValidateServlet.java > PRE-CREATION > docs/src/site/twiki/DG_CommandLineTool.twiki 9494b22 > webapp/src/main/webapp/WEB-INF/web.xml d865d23 > > Diff: https://reviews.apache.org/r/33108/diff/ > > > Testing > ------- > > Unit test. > Manual test. > > > Thanks, > > Azrael Park > >
