You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@oozie.apache.org by Kinga Marton via Review Board <no...@reviews.apache.org> on 2018/07/05 12:50:44 UTC
Review Request 67834: OOZIE-2829 Improve sharelib upload to accept
multiple source folders
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67834/
-----------------------------------------------------------
Review request for oozie, András Piros, Peter Cseh, and Peter Bacsko.
Repository: oozie-git
Description
-------
Right now sharelib can be created via sharelib create -fs FS_URI -locallib SHARED_LIBRARY where the SHARED_LIBRARY can be a tarbal or a folder.
It would be nice to have the possibility to define additional folders to be uploaded into the sharelib, so the users don't have to copy or link the files together on their machine.
The syntax could be something like -additional-lib sharelibName=/path/to/source/;/path/to/some/file,sharelibName2=/path/to/some/folder
Diffs
-----
docs/src/site/twiki/AG_Install.twiki 46363a3b3
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java 75e932c02
tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java d344300ed
tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java d77eba69d
tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java bce0433c6
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java 5929e5ca0
tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java PRE-CREATION
Diff: https://reviews.apache.org/r/67834/diff/1/
Testing
-------
Tested manually + added integration test
Thanks,
Kinga Marton
Re: Review Request 67834: OOZIE-2829 Improve sharelib upload to accept
multiple source folders
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67834/#review206596
-----------------------------------------------------------
tools/src/test/java/org/apache/oozie/tools/IntegrationTestOozieSharelibCLI.java
Lines 46 (patched)
<https://reviews.apache.org/r/67834/#comment289604>
Shouldn't it be rather `@Rule public TemporaryFolder tmpFolder = new TemporaryFolder()`?
tools/src/test/java/org/apache/oozie/tools/IntegrationTestOozieSharelibCLI.java
Lines 82 (patched)
<https://reviews.apache.org/r/67834/#comment289605>
Please provide more context to the assertion error message.
tools/src/test/java/org/apache/oozie/tools/IntegrationTestOozieSharelibCLI.java
Lines 102 (patched)
<https://reviews.apache.org/r/67834/#comment289606>
Please provide more context to the assertion error message.
tools/src/test/java/org/apache/oozie/tools/IntegrationTestOozieSharelibCLI.java
Lines 148 (patched)
<https://reviews.apache.org/r/67834/#comment289607>
Please provide more context to the assertion error message.
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Lines 81-82 (original), 80-81 (patched)
<https://reviews.apache.org/r/67834/#comment289608>
Please provide more context to the assertion error message.
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Line 169 (original), 103 (patched)
<https://reviews.apache.org/r/67834/#comment289609>
Please provide more context to the assertion error message.
- András Piros
On July 25, 2018, 8:48 a.m., Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67834/
> -----------------------------------------------------------
>
> (Updated July 25, 2018, 8:48 a.m.)
>
>
> Review request for oozie, András Piros, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> Right now sharelib can be created via sharelib create -fs FS_URI -locallib SHARED_LIBRARY where the SHARED_LIBRARY can be a tarbal or a folder.
> It would be nice to have the possibility to define additional folders to be uploaded into the sharelib, so the users don't have to copy or link the files together on their machine.
> The syntax could be something like -additional-lib sharelibName=/path/to/source/;/path/to/some/file,sharelibName2=/path/to/some/folder
>
>
> Diffs
> -----
>
> docs/src/site/twiki/AG_Install.twiki 46363a3b3
> tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java 75e932c02
> tools/src/test/java/org/apache/oozie/tools/IntegrationTestOozieSharelibCLI.java PRE-CREATION
> tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java d344300ed
> tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java d77eba69d
> tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java bce0433c6
> tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java 5929e5ca0
> tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIExtraArgsParser.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67834/diff/7/
>
>
> Testing
> -------
>
> Tested manually + added integration test
>
>
> Thanks,
>
> Kinga Marton
>
>
Re: Review Request 67834: OOZIE-2829 Improve sharelib upload to accept
multiple source folders
Posted by Kinga Marton via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67834/
-----------------------------------------------------------
(Updated July 25, 2018, 8:48 a.m.)
Review request for oozie, András Piros, Peter Cseh, and Peter Bacsko.
Changes
-------
multiple options for multiple sharelibs added
Repository: oozie-git
Description
-------
Right now sharelib can be created via sharelib create -fs FS_URI -locallib SHARED_LIBRARY where the SHARED_LIBRARY can be a tarbal or a folder.
It would be nice to have the possibility to define additional folders to be uploaded into the sharelib, so the users don't have to copy or link the files together on their machine.
The syntax could be something like -additional-lib sharelibName=/path/to/source/;/path/to/some/file,sharelibName2=/path/to/some/folder
Diffs (updated)
-----
docs/src/site/twiki/AG_Install.twiki 46363a3b3
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java 75e932c02
tools/src/test/java/org/apache/oozie/tools/IntegrationTestOozieSharelibCLI.java PRE-CREATION
tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java d344300ed
tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java d77eba69d
tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java bce0433c6
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java 5929e5ca0
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIExtraArgsParser.java PRE-CREATION
Diff: https://reviews.apache.org/r/67834/diff/7/
Changes: https://reviews.apache.org/r/67834/diff/6-7/
Testing
-------
Tested manually + added integration test
Thanks,
Kinga Marton
Re: Review Request 67834: OOZIE-2829 Improve sharelib upload to accept
multiple source folders
Posted by Kinga Marton via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67834/
-----------------------------------------------------------
(Updated July 22, 2018, 4 p.m.)
Review request for oozie, András Piros, Peter Cseh, and Peter Bacsko.
Changes
-------
test failure fix
Repository: oozie-git
Description
-------
Right now sharelib can be created via sharelib create -fs FS_URI -locallib SHARED_LIBRARY where the SHARED_LIBRARY can be a tarbal or a folder.
It would be nice to have the possibility to define additional folders to be uploaded into the sharelib, so the users don't have to copy or link the files together on their machine.
The syntax could be something like -additional-lib sharelibName=/path/to/source/;/path/to/some/file,sharelibName2=/path/to/some/folder
Diffs (updated)
-----
docs/src/site/twiki/AG_Install.twiki 46363a3b3
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java 75e932c02
tools/src/test/java/org/apache/oozie/tools/IntegrationTestOozieSharelibCLI.java PRE-CREATION
tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java d344300ed
tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java d77eba69d
tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java bce0433c6
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java 5929e5ca0
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIExtraArgsParser.java PRE-CREATION
Diff: https://reviews.apache.org/r/67834/diff/6/
Changes: https://reviews.apache.org/r/67834/diff/5-6/
Testing
-------
Tested manually + added integration test
Thanks,
Kinga Marton
Re: Review Request 67834: OOZIE-2829 Improve sharelib upload to accept
multiple source folders
Posted by Kinga Marton via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67834/
-----------------------------------------------------------
(Updated July 22, 2018, 1:22 p.m.)
Review request for oozie, András Piros, Peter Cseh, and Peter Bacsko.
Changes
-------
- fixed review comments
- added some additional tests
- changed the sharelibs separator to '+'. However I am not completely satisfied with this solution, but I don't have any better ideas.
Repository: oozie-git
Description
-------
Right now sharelib can be created via sharelib create -fs FS_URI -locallib SHARED_LIBRARY where the SHARED_LIBRARY can be a tarbal or a folder.
It would be nice to have the possibility to define additional folders to be uploaded into the sharelib, so the users don't have to copy or link the files together on their machine.
The syntax could be something like -additional-lib sharelibName=/path/to/source/;/path/to/some/file,sharelibName2=/path/to/some/folder
Diffs (updated)
-----
docs/src/site/twiki/AG_Install.twiki 46363a3b3
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java 75e932c02
tools/src/test/java/org/apache/oozie/tools/IntegrationTestOozieSharelibCLI.java PRE-CREATION
tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java d344300ed
tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java d77eba69d
tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java bce0433c6
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java 5929e5ca0
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIExtraArgsParser.java PRE-CREATION
Diff: https://reviews.apache.org/r/67834/diff/5/
Changes: https://reviews.apache.org/r/67834/diff/4-5/
Testing
-------
Tested manually + added integration test
Thanks,
Kinga Marton
Re: Review Request 67834: OOZIE-2829 Improve sharelib upload to accept
multiple source folders
Posted by Kinga Marton via Review Board <no...@reviews.apache.org>.
> On July 6, 2018, 1:40 p.m., András Piros wrote:
> > tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java
> > Lines 42-48 (original), 43-51 (patched)
> > <https://reviews.apache.org/r/67834/diff/2/?file=2053415#file2053415line43>
> >
> > Could be extracted to a nested `static class`, and tested separately.
I agree with testing it, but this class was created for the Sharelib testing related file operations, like this method.
- Kinga
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67834/#review205796
-----------------------------------------------------------
On July 6, 2018, 8:36 a.m., Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67834/
> -----------------------------------------------------------
>
> (Updated July 6, 2018, 8:36 a.m.)
>
>
> Review request for oozie, András Piros, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> Right now sharelib can be created via sharelib create -fs FS_URI -locallib SHARED_LIBRARY where the SHARED_LIBRARY can be a tarbal or a folder.
> It would be nice to have the possibility to define additional folders to be uploaded into the sharelib, so the users don't have to copy or link the files together on their machine.
> The syntax could be something like -additional-lib sharelibName=/path/to/source/;/path/to/some/file,sharelibName2=/path/to/some/folder
>
>
> Diffs
> -----
>
> docs/src/site/twiki/AG_Install.twiki 46363a3b3
> tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java 75e932c02
> tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java d344300ed
> tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java d77eba69d
> tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java bce0433c6
> tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java 5929e5ca0
> tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67834/diff/4/
>
>
> Testing
> -------
>
> Tested manually + added integration test
>
>
> Thanks,
>
> Kinga Marton
>
>
Re: Review Request 67834: OOZIE-2829 Improve sharelib upload to accept
multiple source folders
Posted by Kinga Marton via Review Board <no...@reviews.apache.org>.
> On July 6, 2018, 1:40 p.m., András Piros wrote:
> > tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
> > Lines 88 (patched)
> > <https://reviews.apache.org/r/67834/diff/2/?file=2053414#file2053414line88>
> >
> > Problem is we can have ```hdfs://my/jar.jar#myjar.jar``` inside sharelib paths.
> >
> > Best is to go with e.g. `;` that cannot be present as HDFS or local file.
>
> Kinga Marton wrote:
> Using semicolon(;) is not a good ideea because the shell handles it as a command separator. What about using & ?
>
> András Piros wrote:
> I'm OK using colon `,` as the separator between sharelib name-path pairs, and pipe `|` between sharelib paths like this:
> ```
> additional-lib sharelibName1=/path/to/source/|/path/to/some/file,sharelibName2=/path/to/some/folder
> ```
The pipe will not work, since it has the same problem as the semicolon. It is interpreted by the Shell. It can work only if is surrounded by double quotes. The problem is that every character that seems to be logic as a delimiter, is interpreted by the Shell.
What about using '+' as sharelibs delimiter?
Using '+' would result in the following command:
```
-extralibs sharelibName=/path/to/source/,/path/to/some/file+sharelibName2=/path/to/some/folder,hdfs://my/jar.jar#myjar.jar
```
Or another solution would be to use double quotes if we want to specify more than one extra sharelib. This way we can even use the semicolon (;)
- Kinga
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67834/#review205796
-----------------------------------------------------------
On July 6, 2018, 8:36 a.m., Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67834/
> -----------------------------------------------------------
>
> (Updated July 6, 2018, 8:36 a.m.)
>
>
> Review request for oozie, András Piros, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> Right now sharelib can be created via sharelib create -fs FS_URI -locallib SHARED_LIBRARY where the SHARED_LIBRARY can be a tarbal or a folder.
> It would be nice to have the possibility to define additional folders to be uploaded into the sharelib, so the users don't have to copy or link the files together on their machine.
> The syntax could be something like -additional-lib sharelibName=/path/to/source/;/path/to/some/file,sharelibName2=/path/to/some/folder
>
>
> Diffs
> -----
>
> docs/src/site/twiki/AG_Install.twiki 46363a3b3
> tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java 75e932c02
> tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java d344300ed
> tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java d77eba69d
> tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java bce0433c6
> tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java 5929e5ca0
> tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67834/diff/4/
>
>
> Testing
> -------
>
> Tested manually + added integration test
>
>
> Thanks,
>
> Kinga Marton
>
>
Re: Review Request 67834: OOZIE-2829 Improve sharelib upload to accept
multiple source folders
Posted by András Piros via Review Board <no...@reviews.apache.org>.
> On July 6, 2018, 1:40 p.m., András Piros wrote:
> > tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
> > Lines 88 (patched)
> > <https://reviews.apache.org/r/67834/diff/2/?file=2053414#file2053414line88>
> >
> > Problem is we can have ```hdfs://my/jar.jar#myjar.jar``` inside sharelib paths.
> >
> > Best is to go with e.g. `;` that cannot be present as HDFS or local file.
>
> Kinga Marton wrote:
> Using semicolon(;) is not a good ideea because the shell handles it as a command separator. What about using & ?
I'm OK using colon `,` as the separator between sharelib name-path pairs, and pipe `|` between sharelib paths like this:
```
additional-lib sharelibName1=/path/to/source/|/path/to/some/file,sharelibName2=/path/to/some/folder
```
- András
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67834/#review205796
-----------------------------------------------------------
On July 6, 2018, 8:36 a.m., Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67834/
> -----------------------------------------------------------
>
> (Updated July 6, 2018, 8:36 a.m.)
>
>
> Review request for oozie, András Piros, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> Right now sharelib can be created via sharelib create -fs FS_URI -locallib SHARED_LIBRARY where the SHARED_LIBRARY can be a tarbal or a folder.
> It would be nice to have the possibility to define additional folders to be uploaded into the sharelib, so the users don't have to copy or link the files together on their machine.
> The syntax could be something like -additional-lib sharelibName=/path/to/source/;/path/to/some/file,sharelibName2=/path/to/some/folder
>
>
> Diffs
> -----
>
> docs/src/site/twiki/AG_Install.twiki 46363a3b3
> tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java 75e932c02
> tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java d344300ed
> tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java d77eba69d
> tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java bce0433c6
> tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java 5929e5ca0
> tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67834/diff/4/
>
>
> Testing
> -------
>
> Tested manually + added integration test
>
>
> Thanks,
>
> Kinga Marton
>
>
Re: Review Request 67834: OOZIE-2829 Improve sharelib upload to accept
multiple source folders
Posted by Kinga Marton via Review Board <no...@reviews.apache.org>.
> On July 6, 2018, 1:40 p.m., András Piros wrote:
> > tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
> > Lines 88 (patched)
> > <https://reviews.apache.org/r/67834/diff/2/?file=2053414#file2053414line88>
> >
> > Problem is we can have ```hdfs://my/jar.jar#myjar.jar``` inside sharelib paths.
> >
> > Best is to go with e.g. `;` that cannot be present as HDFS or local file.
>
> Kinga Marton wrote:
> Using semicolon(;) is not a good ideea because the shell handles it as a command separator. What about using & ?
>
> András Piros wrote:
> I'm OK using colon `,` as the separator between sharelib name-path pairs, and pipe `|` between sharelib paths like this:
> ```
> additional-lib sharelibName1=/path/to/source/|/path/to/some/file,sharelibName2=/path/to/some/folder
> ```
>
> Kinga Marton wrote:
> The pipe will not work, since it has the same problem as the semicolon. It is interpreted by the Shell. It can work only if is surrounded by double quotes. The problem is that every character that seems to be logic as a delimiter, is interpreted by the Shell.
> What about using '+' as sharelibs delimiter?
> Using '+' would result in the following command:
> ```
> -extralibs sharelibName=/path/to/source/,/path/to/some/file+sharelibName2=/path/to/some/folder,hdfs://my/jar.jar#myjar.jar
> ```
>
> Or another solution would be to use double quotes if we want to specify more than one extra sharelib. This way we can even use the semicolon (;)
I have changes the ":" to "+", since I dont't have any better idea. However I am not 100% satisfied with this solution.
- Kinga
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67834/#review205796
-----------------------------------------------------------
On July 6, 2018, 8:36 a.m., Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67834/
> -----------------------------------------------------------
>
> (Updated July 6, 2018, 8:36 a.m.)
>
>
> Review request for oozie, András Piros, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> Right now sharelib can be created via sharelib create -fs FS_URI -locallib SHARED_LIBRARY where the SHARED_LIBRARY can be a tarbal or a folder.
> It would be nice to have the possibility to define additional folders to be uploaded into the sharelib, so the users don't have to copy or link the files together on their machine.
> The syntax could be something like -additional-lib sharelibName=/path/to/source/;/path/to/some/file,sharelibName2=/path/to/some/folder
>
>
> Diffs
> -----
>
> docs/src/site/twiki/AG_Install.twiki 46363a3b3
> tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java 75e932c02
> tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java d344300ed
> tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java d77eba69d
> tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java bce0433c6
> tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java 5929e5ca0
> tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67834/diff/4/
>
>
> Testing
> -------
>
> Tested manually + added integration test
>
>
> Thanks,
>
> Kinga Marton
>
>
Re: Review Request 67834: OOZIE-2829 Improve sharelib upload to accept
multiple source folders
Posted by Kinga Marton via Review Board <no...@reviews.apache.org>.
> On July 6, 2018, 1:40 p.m., András Piros wrote:
> > tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
> > Lines 88 (patched)
> > <https://reviews.apache.org/r/67834/diff/2/?file=2053414#file2053414line88>
> >
> > Problem is we can have ```hdfs://my/jar.jar#myjar.jar``` inside sharelib paths.
> >
> > Best is to go with e.g. `;` that cannot be present as HDFS or local file.
Using semicolon(;) is not a good ideea because the shell handles it as a command separator. What about using & ?
- Kinga
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67834/#review205796
-----------------------------------------------------------
On July 6, 2018, 8:36 a.m., Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67834/
> -----------------------------------------------------------
>
> (Updated July 6, 2018, 8:36 a.m.)
>
>
> Review request for oozie, András Piros, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> Right now sharelib can be created via sharelib create -fs FS_URI -locallib SHARED_LIBRARY where the SHARED_LIBRARY can be a tarbal or a folder.
> It would be nice to have the possibility to define additional folders to be uploaded into the sharelib, so the users don't have to copy or link the files together on their machine.
> The syntax could be something like -additional-lib sharelibName=/path/to/source/;/path/to/some/file,sharelibName2=/path/to/some/folder
>
>
> Diffs
> -----
>
> docs/src/site/twiki/AG_Install.twiki 46363a3b3
> tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java 75e932c02
> tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java d344300ed
> tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java d77eba69d
> tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java bce0433c6
> tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java 5929e5ca0
> tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67834/diff/4/
>
>
> Testing
> -------
>
> Tested manually + added integration test
>
>
> Thanks,
>
> Kinga Marton
>
>
Re: Review Request 67834: OOZIE-2829 Improve sharelib upload to accept
multiple source folders
Posted by Kinga Marton via Review Board <no...@reviews.apache.org>.
> On July 6, 2018, 1:40 p.m., András Piros wrote:
> > tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
> > Lines 282-294 (patched)
> > <https://reviews.apache.org/r/67834/diff/2/?file=2053414#file2053414line294>
> >
> > A log message about how parallel we are running would be nice.
if we are running on multiple threads there is a log message printed out in ConcurrentCopyFromLocal.concurrentCopyFromLocal(FileSystem fs, File srcFile, Path dstPath). Adding here another log message I think it would be redundant.
- Kinga
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67834/#review205796
-----------------------------------------------------------
On July 6, 2018, 8:36 a.m., Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67834/
> -----------------------------------------------------------
>
> (Updated July 6, 2018, 8:36 a.m.)
>
>
> Review request for oozie, András Piros, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> Right now sharelib can be created via sharelib create -fs FS_URI -locallib SHARED_LIBRARY where the SHARED_LIBRARY can be a tarbal or a folder.
> It would be nice to have the possibility to define additional folders to be uploaded into the sharelib, so the users don't have to copy or link the files together on their machine.
> The syntax could be something like -additional-lib sharelibName=/path/to/source/;/path/to/some/file,sharelibName2=/path/to/some/folder
>
>
> Diffs
> -----
>
> docs/src/site/twiki/AG_Install.twiki 46363a3b3
> tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java 75e932c02
> tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java d344300ed
> tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java d77eba69d
> tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java bce0433c6
> tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java 5929e5ca0
> tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67834/diff/4/
>
>
> Testing
> -------
>
> Tested manually + added integration test
>
>
> Thanks,
>
> Kinga Marton
>
>
Re: Review Request 67834: OOZIE-2829 Improve sharelib upload to accept
multiple source folders
Posted by András Piros via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67834/#review205796
-----------------------------------------------------------
docs/src/site/twiki/AG_Install.twiki
Lines 60 (patched)
<https://reviews.apache.org/r/67834/#comment288693>
Typo: comma.
docs/src/site/twiki/AG_Install.twiki
Lines 61 (patched)
<https://reviews.apache.org/r/67834/#comment288695>
Would be better to use mixed separators as per functionality:
```
sharelib-name-1=path-name-1,path-name-2:sharelib-name-2=path-name-1,path-name-2
```
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 74-86 (patched)
<https://reviews.apache.org/r/67834/#comment288694>
Extracting `System.lineSeparator()` to a constant field would increase readability.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 88 (patched)
<https://reviews.apache.org/r/67834/#comment288696>
Problem is we can have ```hdfs://my/jar.jar#myjar.jar``` inside sharelib paths.
Best is to go with e.g. `;` that cannot be present as HDFS or local file.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 264 (patched)
<https://reviews.apache.org/r/67834/#comment288698>
Shouldn't it be `System.out`?
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 282-294 (patched)
<https://reviews.apache.org/r/67834/#comment288699>
A log message about how parallel we are running would be nice.
tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java
Lines 42-48 (original), 43-51 (patched)
<https://reviews.apache.org/r/67834/#comment288701>
Could be extracted to a nested `static class`, and tested separately.
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Lines 181-182 (original), 112-113 (patched)
<https://reviews.apache.org/r/67834/#comment288702>
Multiple testing scenarios missing:
* empty value to a key present
* multiple values to a key
* value without a key
Please use a separate test file.
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Lines 116-118 (patched)
<https://reviews.apache.org/r/67834/#comment288703>
`message` missing.
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Lines 143 (patched)
<https://reviews.apache.org/r/67834/#comment288704>
`message` missing.
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Lines 152 (patched)
<https://reviews.apache.org/r/67834/#comment288705>
`message` missing.
tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java
Lines 84 (patched)
<https://reviews.apache.org/r/67834/#comment288706>
`message` missing.
tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java
Lines 104 (patched)
<https://reviews.apache.org/r/67834/#comment288707>
`message` missing.
tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java
Lines 130 (patched)
<https://reviews.apache.org/r/67834/#comment288708>
`message` missing.
tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java
Lines 147 (patched)
<https://reviews.apache.org/r/67834/#comment288709>
Nice `message` :)
- András Piros
On July 6, 2018, 8:36 a.m., Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67834/
> -----------------------------------------------------------
>
> (Updated July 6, 2018, 8:36 a.m.)
>
>
> Review request for oozie, András Piros, Peter Cseh, and Peter Bacsko.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> Right now sharelib can be created via sharelib create -fs FS_URI -locallib SHARED_LIBRARY where the SHARED_LIBRARY can be a tarbal or a folder.
> It would be nice to have the possibility to define additional folders to be uploaded into the sharelib, so the users don't have to copy or link the files together on their machine.
> The syntax could be something like -additional-lib sharelibName=/path/to/source/;/path/to/some/file,sharelibName2=/path/to/some/folder
>
>
> Diffs
> -----
>
> docs/src/site/twiki/AG_Install.twiki 46363a3b3
> tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java 75e932c02
> tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java d344300ed
> tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java d77eba69d
> tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java bce0433c6
> tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java 5929e5ca0
> tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/67834/diff/4/
>
>
> Testing
> -------
>
> Tested manually + added integration test
>
>
> Thanks,
>
> Kinga Marton
>
>
Re: Review Request 67834: OOZIE-2829 Improve sharelib upload to accept
multiple source folders
Posted by Kinga Marton via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67834/
-----------------------------------------------------------
(Updated July 6, 2018, 8:36 a.m.)
Review request for oozie, András Piros, Peter Cseh, and Peter Bacsko.
Repository: oozie-git
Description
-------
Right now sharelib can be created via sharelib create -fs FS_URI -locallib SHARED_LIBRARY where the SHARED_LIBRARY can be a tarbal or a folder.
It would be nice to have the possibility to define additional folders to be uploaded into the sharelib, so the users don't have to copy or link the files together on their machine.
The syntax could be something like -additional-lib sharelibName=/path/to/source/;/path/to/some/file,sharelibName2=/path/to/some/folder
Diffs (updated)
-----
docs/src/site/twiki/AG_Install.twiki 46363a3b3
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java 75e932c02
tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java d344300ed
tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java d77eba69d
tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java bce0433c6
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java 5929e5ca0
tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java PRE-CREATION
Diff: https://reviews.apache.org/r/67834/diff/2/
Changes: https://reviews.apache.org/r/67834/diff/1-2/
Testing
-------
Tested manually + added integration test
Thanks,
Kinga Marton