[
https://issues.apache.org/jira/browse/OOZIE-2829?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15996719#comment-15996719
]
Peter Bacsko edited comment on OOZIE-2829 at 5/4/17 1:29 PM:
-------------------------------------------------------------
Couple of comments:
- the argument {{-additional_libs}} looks a bit weird to me because of the "_",
isn't there a better name for this? I know it's just nitpicking, but still...
:D Like {{-extralibs}} ?
- the method name {{checkSourceFilesExistance()}} sounds weird too, how about
{{checkIfSourceFilesExist()}}
- couple of protected methods, but only one of them is marked with
@VisibleForTesting - is it necessary to declare them protected?
- throw {{FileNotFoundException}} if a file does not exist (currently
{{IOException}} is thrown)
- Throw a more specific exception here as well (like IllegalArgumentException):
{{throw new Exception(String.format("Additional sharelib, '%s', has been
specified multiple times....}}
- unnecessary comment: {{Integration tests for OozieSharelibCLI}}
- {{SECURITY_MANAGER}} it's not constant, shouldn't be full uppercase
After you addressed these, could you pls create an RB review? It's better to
inspect this patch there.
was (Author: pbacsko):
Couple of comments:
- the argument {{-additional_libs}} looks a bit weird to me because of the "_",
isn't there a better name for this? I know it's just nitpicking, but still...
:D Like {{-extralibs}} ?
- the method name {{checkSourceFilesExistance()}} sounds weird too, how about
{{checkIfSourceFilesExist()}}
- couple of protected methods, but only one of them is marked with
@VisibleForTesting - is it necessary to declare them protected?
- throw {{FileNotFoundException}} if a file does not exist (currently
{{IOException}} is thrown)
- Throw a more specific exception here as well (like IllegalArgumentException):
{{throw new Exception(String.format("Additional sharelib, '%s', has been
specified multiple times....}}
- unnecessary comment: {{Integration tests for OozieSharelibCLI}}
- {{SECURITY_MANAGER}} it's not constant, shouldn't be full uppercase
After you addressed these, could you pls create an RB review? It's better to
inspect this change there.
> Improve sharelib upload to accept multiple source folders
> ---------------------------------------------------------
>
> Key: OOZIE-2829
> URL: https://issues.apache.org/jira/browse/OOZIE-2829
> Project: Oozie
> Issue Type: Bug
> Components: tools
> Reporter: Peter Cseh
> Assignee: Attila Sasvari
> Attachments: OOZIE-2829-01.patch, OOZIE-2829-02.patch,
> OOZIE-2829-03.patch
>
>
> 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
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
