Re: Review Request 67834: OOZIE-2829 Improve sharelib upload to accept multiple source folders

[András Piros via Review Board]

---
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)


Shouldn't it be rather `@Rule public TemporaryFolder tmpFolder = new 
TemporaryFolder()`?



tools/src/test/java/org/apache/oozie/tools/IntegrationTestOozieSharelibCLI.java
Lines 82 (patched)


Please provide more context to the assertion error message.



tools/src/test/java/org/apache/oozie/tools/IntegrationTestOozieSharelibCLI.java
Lines 102 (patched)


Please provide more context to the assertion error message.



tools/src/test/java/org/apache/oozie/tools/IntegrationTestOozieSharelibCLI.java
Lines 148 (patched)


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)


Please provide more context to the assertion error message.



tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Line 169 (original), 103 (patched)


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

[Kinga Marton via Review Board]

---
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

[Kinga Marton via Review Board]

---
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

[Kinga Marton via Review Board]

---
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

[Kinga Marton via Review Board]

> 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)
> > 
> >
> > 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

[Kinga Marton via Review Board]

> 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)
> > 
> >
> > 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

[András Piros via Review Board]

> 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)
> > 
> >
> > 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

[Kinga Marton via Review Board]

> 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)
> > 
> >
> > 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

[Kinga Marton via Review Board]

> 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)
> > 
> >
> > 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

[Kinga Marton via Review Board]

> 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)
> > 
> >
> > 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

[András Piros via Review Board]

---
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)


Typo: comma.



docs/src/site/twiki/AG_Install.twiki
Lines 61 (patched)


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)


Extracting `System.lineSeparator()` to a constant field would increase 
readability.



tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 88 (patched)


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)


Shouldn't it be `System.out`?



tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 282-294 (patched)


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)


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)


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)


`message` missing.



tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Lines 143 (patched)


`message` missing.



tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Lines 152 (patched)


`message` missing.



tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java
Lines 84 (patched)


`message` missing.



tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java
Lines 104 (patched)


`message` missing.



tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java
Lines 130 (patched)


`message` missing.



tools/src/test/java/org/apache/oozie/tools/diag/IntegrationTestOozieSharelibCLI.java
Lines 147 (patched)


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: 

Re: Review Request 67834: OOZIE-2829 Improve sharelib upload to accept multiple source folders

[Kinga Marton via Review Board]

---
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

Review Request 67834: OOZIE-2829 Improve sharelib upload to accept multiple source folders

[Kinga Marton via Review Board]

---
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