> On May 30, 2017, 11:08 a.m., András Piros wrote: > >
I will keep cranking on the documentation but want to get your feedback on the changes so far; I've submitted the current code as a patch to the JIRA. > On May 30, 2017, 11:08 a.m., András Piros wrote: > > core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java > > Lines 53 (patched) > > <https://reviews.apache.org/r/59620/diff/1/?file=1734063#file1734063line53> > > > > `oozie.git.source.uri` would be better. Thanks! I for some reason thought there was a pattern `oozie.<action>.<tree>` but I can not seem to find that anymore and your suggestion makes things much less redundant. > On May 30, 2017, 11:08 a.m., András Piros wrote: > > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java > > Lines 82 (patched) > > <https://reviews.apache.org/r/59620/diff/1/?file=1734068#file1734068line82> > > > > Why not extend `JavaMain` instead? I did not see an advantage over LauncherMain. Is there a reason to derrive from JavaMain? > On May 30, 2017, 11:08 a.m., András Piros wrote: > > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java > > Lines 84-88 (patched) > > <https://reviews.apache.org/r/59620/diff/1/?file=1734068#file1734068line84> > > > > Aren't there existing constants from Apache Hadoop project? I am not finding a clear authoritative sources in the [https://github.com/apache/hadoop](Hadoop) project for `user.name` nor `mapred.*.job.*`; would you have a suggestion? The [JavaActionExecutor](https://github.com/apache/oozie/blob/83d4ddf45aa16649bd9fae367fa915379d5781cd/core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java#L114-L146) defines these all bespoke too sadly. I did find YARN has the resource manager in [https://github.com/apache/hadoop/blob/18c494a00c8ead768f3a868b450dceea485559df/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-api/src/main/java/org/apache/hadoop/yarn/conf/YarnConfiguration.java#L139-L148](YarnConfiguration.java). Would it make more sense to have an Oozie constants class for these? (If so, it looks like this might be a JIRA in its own scope with how often repeated some of these strings are.) > On May 30, 2017, 11:08 a.m., András Piros wrote: > > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java > > Lines 103 (patched) > > <https://reviews.apache.org/r/59620/diff/1/?file=1734068#file1734068line103> > > > > `PROPERTIES_BLACKLIST` sounds better. I'm happy to change this, but `DISALLOWED_PROPERTIES` came from the precident in [JavaActionExecutor](https://github.com/apache/oozie/blob/83d4ddf45aa16649bd9fae367fa915379d5781cd/core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java#L147). > On May 30, 2017, 11:08 a.m., András Piros wrote: > > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java > > Lines 144-145 (patched) > > <https://reviews.apache.org/r/59620/diff/1/?file=1734068#file1734068line144> > > > > Would be goot to `LOG.error()` before re-throwing. What about throwing > > a more action-specific `Exception` w/ the cause caught instead? Cool; I see how JavaMain declares its own exceptions so I'll give that a try. > On May 30, 2017, 11:08 a.m., András Piros wrote: > > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java > > Lines 154 (patched) > > <https://reviews.apache.org/r/59620/diff/1/?file=1734068#file1734068line154> > > > > Is it sure that the target is always HDFS? Can't it be a local path? Good point! Any FileSystem can work; I have updated the code to sound less opinionated as to what FileSystem one is using. > On May 30, 2017, 11:08 a.m., András Piros wrote: > > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java > > Lines 168 (patched) > > <https://reviews.apache.org/r/59620/diff/1/?file=1734068#file1734068line168> > > > > `copyKeyFromHdfs()` would be a better name. Updated to `getKeyFromFS()` > On May 30, 2017, 11:08 a.m., András Piros wrote: > > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java > > Lines 199 (patched) > > <https://reviews.apache.org/r/59620/diff/1/?file=1734068#file1734068line199> > > > > Extract to another class. Please pardon my lack of creativity, however, are you thinking of a repo class perhaps to encapsulate all the repository operations? Or a class for some other reason? > On May 30, 2017, 11:08 a.m., András Piros wrote: > > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java > > Lines 254 (patched) > > <https://reviews.apache.org/r/59620/diff/1/?file=1734068#file1734068line254> > > > > Extract to another class. Similarly, would this fit better in a repo class (if split off independently) or independent of the repo code and `GitMain`? > On May 30, 2017, 11:08 a.m., András Piros wrote: > > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java > > Lines 125-128 (patched) > > <https://reviews.apache.org/r/59620/diff/1/?file=1734070#file1734070line125> > > > > Extract method. Moved this to a `setUp()` method. > On May 30, 2017, 11:08 a.m., András Piros wrote: > > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java > > Lines 60 (patched) > > <https://reviews.apache.org/r/59620/diff/1/?file=1734071#file1734071line60> > > > > Are system props reset after this test run? I am not sure what the entire chain of `ActionExecutorTestCase`'s methods are doing. However, I am not explicitly clearing this to my knowledge; do you expect issues? (For reference, it seems [`TestJavaActionExecutor()`](https://github.com/apache/oozie/blob/5998c18fde1da769e91e3ef1bcca484723730c76/core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java#L90) is not clearing these either?) > On May 30, 2017, 11:08 a.m., András Piros wrote: > > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java > > Lines 80 (patched) > > <https://reviews.apache.org/r/59620/diff/1/?file=1734071#file1734071line80> > > > > Please cut this into independent test cases, and name these accordingly. Could you provide me more direction on how you would ike this method separated? Since the assertions in this method depend on the run state of the action under test, I would need to run the action many times (all with the same configuration); to me that would be a single case usually? > On May 30, 2017, 11:08 a.m., András Piros wrote: > > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java > > Lines 153 (patched) > > <https://reviews.apache.org/r/59620/diff/1/?file=1734071#file1734071line153> > > > > Extract to nested class. Would you have an example in the codebase I could emulate? It looks like I am following the same pattern as: `./sharelib/spark/src/test/java/org/apache/oozie/action/hadoop/TestSparkActionExecutor.java` `./sharelib/sqoop/src/test/java/org/apache/oozie/action/hadoop/TestSqoopActionExecutor.java` `./sharelib/hive2/src/test/java/org/apache/oozie/action/hadoop/TestHive2ActionExecutor.java` Perhaps a separate JIRA to refactor all the test cases would be better if this would be a new pattern? - Clay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59620/#review176291 ----------------------------------------------------------- On May 30, 2017, 12:03 a.m., Clay B. wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59620/ > ----------------------------------------------------------- > > (Updated May 30, 2017, 12:03 a.m.) > > > Review request for oozie and András Piros. > > > Bugs: OOZIE-2877 > https://issues.apache.org/jira/browse/OOZIE-2877 > > > Repository: oozie-git > > > Description > ------- > > OOZIE-2877 - Oozie Git Action > > > Diffs > ----- > > client/src/main/java/org/apache/oozie/cli/OozieCLI.java 38fb84e > client/src/main/resources/git-action-0.1.xsd PRE-CREATION > core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java > PRE-CREATION > core/src/main/resources/oozie-default.xml 076401d > docs/src/site/twiki/WorkflowFunctionalSpec.twiki 6655696 > pom.xml ebe1d68 > sharelib/git/pom.xml PRE-CREATION > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java > PRE-CREATION > > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java > PRE-CREATION > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java > PRE-CREATION > > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java > PRE-CREATION > sharelib/pom.xml 190bd04 > src/main/assemblies/sharelib.xml ea95c2e > webapp/pom.xml e4fdfb7 > > > Diff: https://reviews.apache.org/r/59620/diff/1/ > > > Testing > ------- > > Tested using unit and integration tests. Still need to: > * Test on a cluster > * Test with an authenticated SSH hosted Git repo > > Sumitted a request to the JGit community as their branch pulling code seems > to have an > [issue](https://dev.eclipse.org/mhonarc/lists/jgit-dev/msg03343.html). > > > File Attachments > ---------------- > > 0001-OOZIE-2877-Oozie-Git-Action.patch > > https://reviews.apache.org/media/uploaded/files/2017/05/29/24f90a78-3dc1-49fe-bf29-5927a3cd5e72__0001-OOZIE-2877-Oozie-Git-Action.patch > Patch > > https://reviews.apache.org/media/uploaded/files/2017/05/29/dd23dd72-67e0-456f-9b52-e566d8d17d16__0001-OOZIE-2877-Oozie-Git-Action.patch > > > Thanks, > > Clay B. > >
