----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59620/#review176291 -----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java Lines 53 (patched) <https://reviews.apache.org/r/59620/#comment249652> `oozie.git.source.uri` would be better. core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java Lines 54 (patched) <https://reviews.apache.org/r/59620/#comment249653> `oozie.git.branch` would be better. core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java Lines 73 (patched) <https://reviews.apache.org/r/59620/#comment249654> What about throwing an `ActionExecutorException` instead? core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java Lines 110-114 (patched) <https://reviews.apache.org/r/59620/#comment249655> Extract method. core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java Lines 116-120 (patched) <https://reviews.apache.org/r/59620/#comment249656> Extract method. core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java Lines 122-126 (patched) <https://reviews.apache.org/r/59620/#comment249657> Extract method. core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java Lines 130-134 (patched) <https://reviews.apache.org/r/59620/#comment249658> Extract method. core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java Lines 138-142 (patched) <https://reviews.apache.org/r/59620/#comment249659> Extract method. core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java Lines 154-158 (patched) <https://reviews.apache.org/r/59620/#comment249660> Extract method. core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java Lines 161-164 (patched) <https://reviews.apache.org/r/59620/#comment249661> Extract method. core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java Lines 166-169 (patched) <https://reviews.apache.org/r/59620/#comment249662> Extract method. docs/src/site/twiki/WorkflowFunctionalSpec.twiki Lines 1617 (patched) <https://reviews.apache.org/r/59620/#comment249663> Please resolve this TODO. docs/src/site/twiki/WorkflowFunctionalSpec.twiki Lines 1621 (patched) <https://reviews.apache.org/r/59620/#comment249664> Please resolve this TODO. docs/src/site/twiki/WorkflowFunctionalSpec.twiki Lines 1624 (patched) <https://reviews.apache.org/r/59620/#comment249665> Please resolve this TODO. sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 82 (patched) <https://reviews.apache.org/r/59620/#comment249666> Why not extend `JavaMain` instead? sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 84-88 (patched) <https://reviews.apache.org/r/59620/#comment249667> Aren't there existing constants from Apache Hadoop project? sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 103 (patched) <https://reviews.apache.org/r/59620/#comment249668> `PROPERTIES_BLACKLIST` sounds better. sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 129 (patched) <https://reviews.apache.org/r/59620/#comment249669> Sounds like a good idea extracting `oozie.action.conf.xml` to a constant. sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 144-145 (patched) <https://reviews.apache.org/r/59620/#comment249670> Would be goot to `LOG.error()` before re-throwing. What about throwing a more action-specific `Exception` w/ the cause caught instead? sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 154 (patched) <https://reviews.apache.org/r/59620/#comment249672> Is it sure that the target is always HDFS? Can't it be a local path? sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 156-157 (patched) <https://reviews.apache.org/r/59620/#comment249671> Would be goot to `LOG.error()` before re-throwing. What about throwing a more action-specific `Exception` w/ the cause caught instead? sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 168 (patched) <https://reviews.apache.org/r/59620/#comment249675> `copyKeyFromHdfs()` would be a better name. sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 179-182 (patched) <https://reviews.apache.org/r/59620/#comment249676> Better to extract a `String` variable and reuse. sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 186-187 (patched) <https://reviews.apache.org/r/59620/#comment249677> Better to extract a `String` variable and reuse. sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 199 (patched) <https://reviews.apache.org/r/59620/#comment249678> Extract to another class. sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 254 (patched) <https://reviews.apache.org/r/59620/#comment249679> Extract to another class. sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 299-303 (patched) <https://reviews.apache.org/r/59620/#comment249680> Extract method. sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 305-309 (patched) <https://reviews.apache.org/r/59620/#comment249681> Extract method. sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 311-315 (patched) <https://reviews.apache.org/r/59620/#comment249682> Extract method. sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 317-321 (patched) <https://reviews.apache.org/r/59620/#comment249683> Extract method. sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 323-327 (patched) <https://reviews.apache.org/r/59620/#comment249684> Extract method. sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 329-333 (patched) <https://reviews.apache.org/r/59620/#comment249685> Extract method. sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 334-345 (patched) <https://reviews.apache.org/r/59620/#comment249687> Extract method. sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 347-351 (patched) <https://reviews.apache.org/r/59620/#comment249686> Extract method. sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 352-361 (patched) <https://reviews.apache.org/r/59620/#comment249688> Extract method. sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 370-374 (patched) <https://reviews.apache.org/r/59620/#comment249689> Extract method. sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java Lines 376-380 (patched) <https://reviews.apache.org/r/59620/#comment249690> Extract method. sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java Lines 102-114 (patched) <https://reviews.apache.org/r/59620/#comment249691> Pls. remove dead code. sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java Lines 117-121 (patched) <https://reviews.apache.org/r/59620/#comment249692> Pls. remove dead code. sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java Lines 125-128 (patched) <https://reviews.apache.org/r/59620/#comment249695> Extract method. sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java Lines 131-133 (patched) <https://reviews.apache.org/r/59620/#comment249696> Please employ that `GitMain.nameNode` is (package-)protected instead, and use `@VisibleForTesting`. sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java Lines 136 (patched) <https://reviews.apache.org/r/59620/#comment249693> Pls. remove dead code. sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java Lines 138-144 (patched) <https://reviews.apache.org/r/59620/#comment249697> Use try-with-resources instead. sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMain.java Lines 146-187 (patched) <https://reviews.apache.org/r/59620/#comment249694> Pls. remove dead code. sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java Lines 60 (patched) <https://reviews.apache.org/r/59620/#comment249698> Are system props reset after this test run? sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java Lines 80 (patched) <https://reviews.apache.org/r/59620/#comment249699> Please cut this into independent test cases, and name these accordingly. sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java Lines 153 (patched) <https://reviews.apache.org/r/59620/#comment249700> Extract to nested class. - András Piros 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. > >
