You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by Peter Bacsko via Review Board <no...@reviews.apache.org> on 2018/07/02 12:46:54 UTC
Re: Review Request 59620: This review board request is for an action
to provide a Git action for Oozie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review205636
-----------------------------------------------------------
core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 95 (patched)
<https://reviews.apache.org/r/59620/#comment288510>
Is this package private for a reason?
core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
Lines 226-227 (patched)
<https://reviews.apache.org/r/59620/#comment288509>
We shouldn't catch NPE let alone ignore it.
docs/src/site/twiki/WorkflowFunctionalSpec.twiki
Lines 1667-1669 (patched)
<https://reviews.apache.org/r/59620/#comment288515>
Do we really need to access a different cluster other than the one we're running on?
If key is located on a separate cluster which is secure we won't be allowed to access that file without proper credentials. Basically it's the same problem that appears under disctp. It's really not self-explanatory to setup cross-cluster authentication.
Therefore I'm against it. Also, the name node is available from "fs.defaultFS" property.
sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 41 (patched)
<https://reviews.apache.org/r/59620/#comment288508>
We don't use XLog inside LauncherMain, only Sysout. This will change in the future, but for now, let's stick to the conventions that we have already.
sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 166 (patched)
<https://reviews.apache.org/r/59620/#comment288513>
A better method name would be something like isValidUri()
sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 185-188 (patched)
<https://reviews.apache.org/r/59620/#comment288512>
Is it necessary to validate these separately? If anything, these values should be checked inside Oozie, not when the container is alreay running.
sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
Lines 201-204 (patched)
<https://reviews.apache.org/r/59620/#comment288514>
If we don't allow relative URIs (null scheme), then this check should be placed inside validUri().
sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java
Lines 54-56 (patched)
<https://reviews.apache.org/r/59620/#comment288516>
Would be better to have these as final
sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java
Lines 82-99 (patched)
<https://reviews.apache.org/r/59620/#comment288517>
We should not clone repositories under an unit test. I suggest using a Jsch mock for testing.
There are different ways to achieve this. I can think of having a property like "oozie.git.operations.class" which defaults to this class but we can override it to a different implementation which does not use JSch.
sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
Lines 113 (patched)
<https://reviews.apache.org/r/59620/#comment288506>
Use Assert.fail() to indicate failure
sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitActionExecutor.java
Lines 116 (patched)
<https://reviews.apache.org/r/59620/#comment288507>
Assert.fail()
sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestGitMainGetKey.java
Lines 51-56 (patched)
<https://reviews.apache.org/r/59620/#comment288520>
Simplify this part, see related comment below
sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
Lines 43 (patched)
<https://reviews.apache.org/r/59620/#comment288518>
As I mentioned above, let's avoid cloning a repository during test execution.
sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
Lines 74-80 (patched)
<https://reviews.apache.org/r/59620/#comment288519>
This is way too complicated. Do we just read a contents of a file?
There are simpler means to do this: https://howtodoinjava.com/core-java/io/java-read-file-to-string-examples/
sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
Lines 119-122 (patched)
<https://reviews.apache.org/r/59620/#comment288521>
Three things:
1. We don't need JobConf anymore because we don't launch the action as a MapReduce action. Just use the plain Configuration class which JobConf extends.
2. No need to set "mapred.job.tracker", it's a deprecated property anyway
3. createJobClient() creates an MR job client, but we don't need it, plus the return value is ignored.
- Peter Bacsko
On jún. 26, 2018, 9:50 du, Clay B. wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> -----------------------------------------------------------
>
> (Updated jún. 26, 2018, 9:50 du)
>
>
> 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/resources/git-action-1.0.xsd PRE-CREATION
> core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java PRE-CREATION
> core/src/main/resources/oozie-default.xml ff1820c
> core/src/test/java/org/apache/oozie/test/XTestCase.java 661970d
> docs/src/site/twiki/WorkflowFunctionalSpec.twiki 76cbe21
> examples/src/main/apps/git/job.properties PRE-CREATION
> examples/src/main/apps/git/workflow.xml PRE-CREATION
> pom.xml 0c39d64
> sharelib/git/pom.xml PRE-CREATION
> sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java PRE-CREATION
> sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.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 6a0864d
> src/main/assemblies/sharelib.xml 07dc69c
> webapp/pom.xml 4a32b54
>
>
> Diff: https://reviews.apache.org/r/59620/diff/7/
>
>
> 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.
>
>
Re: Review Request 59620: This review board request is for an action
to provide a Git action for Oozie
Posted by "Clay B." <cw...@clayb.net>.
> On July 2, 2018, 12:46 p.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java
> > Lines 95 (patched)
> > <https://reviews.apache.org/r/59620/diff/7/?file=2046211#file2046211line95>
> >
> > Is this package private for a reason?
No, simply omission. Thanks for finding this!
> On July 2, 2018, 12:46 p.m., Peter Bacsko wrote:
> > docs/src/site/twiki/WorkflowFunctionalSpec.twiki
> > Lines 1667-1669 (patched)
> > <https://reviews.apache.org/r/59620/diff/7/?file=2046214#file2046214line1667>
> >
> > Do we really need to access a different cluster other than the one we're running on?
> >
> > If key is located on a separate cluster which is secure we won't be allowed to access that file without proper credentials. Basically it's the same problem that appears under disctp. It's really not self-explanatory to setup cross-cluster authentication.
> >
> > Therefore I'm against it. Also, the name node is available from "fs.defaultFS" property.
I could certainly see someone storing a credential on a secure object store (e.g. S3-like FS).
> On July 2, 2018, 12:46 p.m., Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 41 (patched)
> > <https://reviews.apache.org/r/59620/diff/7/?file=2046219#file2046219line41>
> >
> > We don't use XLog inside LauncherMain, only Sysout. This will change in the future, but for now, let's stick to the conventions that we have already.
I have left the help methods to leave logging level intention in place but removed all references to XLog. Would you prefer the helpers to go too?
> On July 2, 2018, 12:46 p.m., Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 166 (patched)
> > <https://reviews.apache.org/r/59620/diff/7/?file=2046219#file2046219line166>
> >
> > A better method name would be something like isValidUri()
Sounds good
> On July 2, 2018, 12:46 p.m., Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 185-188 (patched)
> > <https://reviews.apache.org/r/59620/diff/7/?file=2046219#file2046219line185>
> >
> > Is it necessary to validate these separately? If anything, these values should be checked inside Oozie, not when the container is alreay running.
I have removed these checks.
> On July 2, 2018, 12:46 p.m., Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 201-204 (patched)
> > <https://reviews.apache.org/r/59620/diff/7/?file=2046219#file2046219line201>
> >
> > If we don't allow relative URIs (null scheme), then this check should be placed inside validUri().
Integrated into isValidURI.
> On July 2, 2018, 12:46 p.m., Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java
> > Lines 82-99 (patched)
> > <https://reviews.apache.org/r/59620/diff/7/?file=2046220#file2046220line82>
> >
> > We should not clone repositories under an unit test. I suggest using a Jsch mock for testing.
> >
> > There are different ways to achieve this. I can think of having a property like "oozie.git.operations.class" which defaults to this class but we can override it to a different implementation which does not use JSch.
I have found a few descriptions on how to build a unit-test server in JGit; now doing that.
> On July 2, 2018, 12:46 p.m., Peter Bacsko wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
> > Lines 43 (patched)
> > <https://reviews.apache.org/r/59620/diff/7/?file=2046223#file2046223line43>
> >
> > As I mentioned above, let's avoid cloning a repository during test execution.
Still clones but from an in-JVM server.
> On July 2, 2018, 12:46 p.m., Peter Bacsko wrote:
> > sharelib/git/src/test/java/org/apache/oozie/action/hadoop/TestIntegrationGitActionExecutor.java
> > Lines 119-122 (patched)
> > <https://reviews.apache.org/r/59620/diff/7/?file=2046223#file2046223line119>
> >
> > Three things:
> >
> > 1. We don't need JobConf anymore because we don't launch the action as a MapReduce action. Just use the plain Configuration class which JobConf extends.
> >
> > 2. No need to set "mapred.job.tracker", it's a deprecated property anyway
> >
> > 3. createJobClient() creates an MR job client, but we don't need it, plus the return value is ignored.
I still use createJobConf from XTestCase to keep uniformity with other test cases but I use a Configuration instead of a JobConf now.
- Clay
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review205636
-----------------------------------------------------------
On June 26, 2018, 9:50 p.m., Clay B. wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> -----------------------------------------------------------
>
> (Updated June 26, 2018, 9:50 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/resources/git-action-1.0.xsd PRE-CREATION
> core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java PRE-CREATION
> core/src/main/resources/oozie-default.xml ff1820c
> core/src/test/java/org/apache/oozie/test/XTestCase.java 661970d
> docs/src/site/twiki/WorkflowFunctionalSpec.twiki 76cbe21
> examples/src/main/apps/git/job.properties PRE-CREATION
> examples/src/main/apps/git/workflow.xml PRE-CREATION
> pom.xml 0c39d64
> sharelib/git/pom.xml PRE-CREATION
> sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java PRE-CREATION
> sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.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 6a0864d
> src/main/assemblies/sharelib.xml 07dc69c
> webapp/pom.xml 4a32b54
>
>
> Diff: https://reviews.apache.org/r/59620/diff/7/
>
>
> 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.
>
>
Re: Review Request 59620: This review board request is for an action
to provide a Git action for Oozie
Posted by Peter Bacsko via Review Board <no...@reviews.apache.org>.
> On júl. 2, 2018, 12:46 du, Peter Bacsko wrote:
> > docs/src/site/twiki/WorkflowFunctionalSpec.twiki
> > Lines 1667-1669 (patched)
> > <https://reviews.apache.org/r/59620/diff/7/?file=2046214#file2046214line1667>
> >
> > Do we really need to access a different cluster other than the one we're running on?
> >
> > If key is located on a separate cluster which is secure we won't be allowed to access that file without proper credentials. Basically it's the same problem that appears under disctp. It's really not self-explanatory to setup cross-cluster authentication.
> >
> > Therefore I'm against it. Also, the name node is available from "fs.defaultFS" property.
>
> Clay B. wrote:
> I could certainly see someone storing a credential on a secure object store (e.g. S3-like FS).
In that case, we have to mention in the docs that access control on the remote side must be properly configured. Eg. if it's a hdfs:// URL that points to an non-secure cluster, appropriate HDFS permissions are still necessary.
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review205636
-----------------------------------------------------------
On aug. 3, 2018, 10 du, Clay B. wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> -----------------------------------------------------------
>
> (Updated aug. 3, 2018, 10 du)
>
>
> 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/resources/git-action-1.0.xsd PRE-CREATION
> core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java PRE-CREATION
> core/src/main/resources/oozie-default.xml b69d2c9
> core/src/test/java/org/apache/oozie/test/XTestCase.java 661970d
> docs/src/site/twiki/WorkflowFunctionalSpec.twiki 76cbe21
> examples/src/main/apps/git/job.properties PRE-CREATION
> examples/src/main/apps/git/workflow.xml PRE-CREATION
> fluent-job/fluent-job-api/pom.xml 4c9b853
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java PRE-CREATION
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java PRE-CREATION
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java 7bb82e5
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java PRE-CREATION
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java PRE-CREATION
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java ec56554
> fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e
> fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890
> pom.xml 92358aa
> sharelib/git/pom.xml PRE-CREATION
> sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java PRE-CREATION
> sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java PRE-CREATION
> sharelib/git/src/test/java/org/apache/oozie/action/hadoop/GitServer.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 6a0864d
> src/main/assemblies/sharelib.xml 07dc69c
> webapp/pom.xml fd3f89f
>
>
> Diff: https://reviews.apache.org/r/59620/diff/10/
>
>
> 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.
>
>
Re: Review Request 59620: This review board request is for an action
to provide a Git action for Oozie
Posted by Peter Bacsko via Review Board <no...@reviews.apache.org>.
> On júl. 2, 2018, 12:46 du, Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 41 (patched)
> > <https://reviews.apache.org/r/59620/diff/7/?file=2046219#file2046219line41>
> >
> > We don't use XLog inside LauncherMain, only Sysout. This will change in the future, but for now, let's stick to the conventions that we have already.
>
> Clay B. wrote:
> I have left the help methods to leave logging level intention in place but removed all references to XLog. Would you prefer the helpers to go too?
We haven't settled down on a logging solution. Geza suggested JDK loggers (so that they don't conflict with log4j/log4j2/logback, whatever we migrate to), but it's not finalized yet.
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review205636
-----------------------------------------------------------
On aug. 3, 2018, 10 du, Clay B. wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> -----------------------------------------------------------
>
> (Updated aug. 3, 2018, 10 du)
>
>
> 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/resources/git-action-1.0.xsd PRE-CREATION
> core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java PRE-CREATION
> core/src/main/resources/oozie-default.xml b69d2c9
> core/src/test/java/org/apache/oozie/test/XTestCase.java 661970d
> docs/src/site/twiki/WorkflowFunctionalSpec.twiki 76cbe21
> examples/src/main/apps/git/job.properties PRE-CREATION
> examples/src/main/apps/git/workflow.xml PRE-CREATION
> fluent-job/fluent-job-api/pom.xml 4c9b853
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java PRE-CREATION
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java PRE-CREATION
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java 7bb82e5
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java PRE-CREATION
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java PRE-CREATION
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java ec56554
> fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e
> fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890
> pom.xml 92358aa
> sharelib/git/pom.xml PRE-CREATION
> sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java PRE-CREATION
> sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java PRE-CREATION
> sharelib/git/src/test/java/org/apache/oozie/action/hadoop/GitServer.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 6a0864d
> src/main/assemblies/sharelib.xml 07dc69c
> webapp/pom.xml fd3f89f
>
>
> Diff: https://reviews.apache.org/r/59620/diff/10/
>
>
> 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.
>
>
Re: Review Request 59620: This review board request is for an action
to provide a Git action for Oozie
Posted by András Piros via Review Board <no...@reviews.apache.org>.
> On July 2, 2018, 12:46 p.m., Peter Bacsko wrote:
> > docs/src/site/twiki/WorkflowFunctionalSpec.twiki
> > Lines 1667-1669 (patched)
> > <https://reviews.apache.org/r/59620/diff/7/?file=2046214#file2046214line1667>
> >
> > Do we really need to access a different cluster other than the one we're running on?
> >
> > If key is located on a separate cluster which is secure we won't be allowed to access that file without proper credentials. Basically it's the same problem that appears under disctp. It's really not self-explanatory to setup cross-cluster authentication.
> >
> > Therefore I'm against it. Also, the name node is available from "fs.defaultFS" property.
>
> Clay B. wrote:
> I could certainly see someone storing a credential on a secure object store (e.g. S3-like FS).
>
> Peter Bacsko wrote:
> In that case, we have to mention in the docs that access control on the remote side must be properly configured. Eg. if it's a hdfs:// URL that points to an non-secure cluster, appropriate HDFS permissions are still necessary.
A paragraph added to the docs.
> On July 2, 2018, 12:46 p.m., Peter Bacsko wrote:
> > sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java
> > Lines 41 (patched)
> > <https://reviews.apache.org/r/59620/diff/7/?file=2046219#file2046219line41>
> >
> > We don't use XLog inside LauncherMain, only Sysout. This will change in the future, but for now, let's stick to the conventions that we have already.
>
> Clay B. wrote:
> I have left the help methods to leave logging level intention in place but removed all references to XLog. Would you prefer the helpers to go too?
>
> Peter Bacsko wrote:
> We haven't settled down on a logging solution. Geza suggested JDK loggers (so that they don't conflict with log4j/log4j2/logback, whatever we migrate to), but it's not finalized yet.
`XLog` references are not more present in `GitMain`.
- András
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59620/#review205636
-----------------------------------------------------------
On Aug. 3, 2018, 10 p.m., Clay B. wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59620/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2018, 10 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/resources/git-action-1.0.xsd PRE-CREATION
> core/src/main/java/org/apache/oozie/action/hadoop/GitActionExecutor.java PRE-CREATION
> core/src/main/resources/oozie-default.xml b69d2c9
> core/src/test/java/org/apache/oozie/test/XTestCase.java 661970d
> docs/src/site/twiki/WorkflowFunctionalSpec.twiki 76cbe21
> examples/src/main/apps/git/job.properties PRE-CREATION
> examples/src/main/apps/git/workflow.xml PRE-CREATION
> fluent-job/fluent-job-api/pom.xml 4c9b853
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitAction.java PRE-CREATION
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/action/GitActionBuilder.java PRE-CREATION
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/ExplicitNodeConverter.java 7bb82e5
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitConfigurationConverter.java PRE-CREATION
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/mapping/GitPrepareConverter.java PRE-CREATION
> fluent-job/fluent-job-api/src/main/java/org/apache/oozie/fluentjob/api/serialization/WorkflowMarshaller.java ec56554
> fluent-job/fluent-job-api/src/main/resources/action_mappings.xml a5f890e
> fluent-job/fluent-job-api/src/main/xjb/bindings.xml 48f6890
> pom.xml 92358aa
> sharelib/git/pom.xml PRE-CREATION
> sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitMain.java PRE-CREATION
> sharelib/git/src/main/java/org/apache/oozie/action/hadoop/GitOperations.java PRE-CREATION
> sharelib/git/src/test/java/org/apache/oozie/action/hadoop/GitServer.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 6a0864d
> src/main/assemblies/sharelib.xml 07dc69c
> webapp/pom.xml fd3f89f
>
>
> Diff: https://reviews.apache.org/r/59620/diff/10/
>
>
> 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.
>
>