> 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? > > Clay B. wrote: > 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.)
`"user.name"` -> `Hive2Credentials.USER_NAME`, `OozieClient.USER_NAME` For the MR1 config keys there isn't a current one in the Hadoop repo, for the newer ones: `"mapreduce.jobtracker.address"` -> [`MRConfig.MASTER_ADDRESS`](https://github.com/apache/hadoop/blob/f67237cbe7bc48a1b9088e990800b37529f1db2a/hadoop-mapreduce-project/hadoop-mapreduce-client/hadoop-mapreduce-client-core/src/main/java/org/apache/hadoop/mapreduce/MRConfig.java#L65) I think less dependencies are always better, I'm for introducing these non-Oozie constants in `GitMain`, and reusing Oozie constants from code inside Oozie. > 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. > > Clay B. wrote: > 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? I'm thinking of a `GitRepositoryOperations` class to encapsulate all the repository operation, having fields like `gitSrc`, `branch`, `outputDir`, `credentialFile` initialized in constructor. > 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. > > Clay B. wrote: > Similarly, would this fit better in a repo class (if split off > independently) or independent of the repo code and `GitMain`? I'm thinking of a `GitRepositoryOperations` class to encapsulate all the repository operation, having fields like `gitSrc`, `branch`, `outputDir`, `credentialFile` initialized in constructor. > 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? > > Clay B. wrote: > 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?) I'd rather not set any system properties here but go for [`Services.get().getConf().set()`](https://github.com/apache/oozie/blob/eb168360c550943828d9fe2c7bfdcd4f5e830003/core/src/test/java/org/apache/oozie/service/TestActionService.java#L52-L53), if that works. If not, I'd still set this system property in `@Before void setUp()` and reset in `@After void tearDown()` as this one is used by other important pieces of code. > 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. > > Clay B. wrote: > 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? The current unit tests are more readable, closing this. > 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. > > Clay B. wrote: > 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? True, same pattern, sadly :) closing this one as well. - András ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/59620/#review176291 ----------------------------------------------------------- On May 4, 2018, 5:43 p.m., Clay B. wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/59620/ > ----------------------------------------------------------- > > (Updated May 4, 2018, 5:43 p.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 > 4abc7502c0c9d8b59ded2baaed30c407ad073008 > 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 > 6f8925828090cee29a818de30a7c31db0617d816 > docs/src/site/twiki/WorkflowFunctionalSpec.twiki > 76cbe21ecd79eae86885a8db77831bf2c40f2d4f > examples/src/main/apps/git/job.properties PRE-CREATION > examples/src/main/apps/git/workflow.xml PRE-CREATION > pom.xml d9fe1b20f571b1a6afacb3ce53f4d409dd3d60b1 > 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/TestGitMainGetKey.java > PRE-CREATION > > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java > PRE-CREATION > sharelib/pom.xml 6a0864d4e5b52160dc4e1a5cb9dfc34a037b67c8 > src/main/assemblies/sharelib.xml 07dc69c8276895b254e5af0cc021cce6ebad18f4 > webapp/pom.xml 797996912b6e6381b261a69f8eb1e012fe488fdf > > > Diff: https://reviews.apache.org/r/59620/diff/5/ > > > 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). > > Andras has tested against his patch 011 with results at > https://issues.apache.org/jira/browse/OOZIE-2877?focusedCommentId=16459523&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-16459523 > > > 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. > >
