You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@oozie.apache.org by Mate Juhasz <ma...@gmail.com> on 2019/03/01 14:44:14 UTC

Re: Review Request 66656: Exclusion pattern for sharelib.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66656/
-----------------------------------------------------------

(Updated March 1, 2019, 2:44 p.m.)


Review request for oozie, András Piros and Denes Bodo.


Changes
-------

rebased to master


Bugs: OOZIE-1624
    https://issues.apache.org/jira/browse/OOZIE-1624


Repository: oozie-git


Description
-------

OOZIE-1624 Exclusion pattern for sharelib.


Diffs (updated)
-----

  core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 231b38ea 
  core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java PRE-CREATION 
  core/src/main/java/org/apache/oozie/service/ShareLibService.java b88dab3a 
  core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 05511e4c 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 6383e814 
  core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java PRE-CREATION 
  core/src/test/java/org/apache/oozie/action/hadoop/TestShareLibExcluder.java PRE-CREATION 


Diff: https://reviews.apache.org/r/66656/diff/6/

Changes: https://reviews.apache.org/r/66656/diff/5-6/


Testing
-------

Tested on a cluster


Thanks,

Mate Juhasz

Re: Review Request 66656: Exclusion pattern for sharelib.

Posted by Andras Salamon <an...@melda.info>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66656/#review213386
-----------------------------------------------------------




core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 176-177 (patched)
<https://reviews.apache.org/r/66656/#comment299306>

    Could you please use 'static final' instead of 'final static' to keep it consistent.



core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 715 (patched)
<https://reviews.apache.org/r/66656/#comment299307>

    Please add NullPointer check for files



core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 755 (patched)
<https://reviews.apache.org/r/66656/#comment299308>

    please insert line break to follow the Oozie coding style



core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 757 (patched)
<https://reviews.apache.org/r/66656/#comment299309>

    grammar: Failed to acquire



core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 760 (patched)
<https://reviews.apache.org/r/66656/#comment299326>

    Please delete this comment. If it has some valueable information, then extend the log message.



core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 779 (patched)
<https://reviews.apache.org/r/66656/#comment299310>

    Do we really need this comment? 
    
    APP_LIB_PATH_LIST = "oozie.wf.application.lib"
    
    so I don't see the added value.



core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 1172 (patched)
<https://reviews.apache.org/r/66656/#comment299327>

    please insert line break to follow the Oozie coding style



core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 1175 (patched)
<https://reviews.apache.org/r/66656/#comment299311>

    please insert line break to follow the Oozie coding style



core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java
Lines 41-42 (patched)
<https://reviews.apache.org/r/66656/#comment299329>

    What happens if shareLibRoot is null? 
    
    It can be null because getSharelibRoot() can return null.
    
    In this case we do lots of initialization but shouldExclude will return false.
    
    Do I understand it correctly? Do we need the initialization in this case?



core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java
Lines 43 (patched)
<https://reviews.apache.org/r/66656/#comment299312>

    Could we use the same variable name? What is the reason for the rootURI/shareLibRoot name?



core/src/main/java/org/apache/oozie/service/ShareLibService.java
Lines 624 (patched)
<https://reviews.apache.org/r/66656/#comment299313>

    Please add some error message to eliminate javadoc warnings.



core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java
Lines 518 (patched)
<https://reviews.apache.org/r/66656/#comment299320>

    I'd rather create two separate methods instead of the boolean contains variable.
    
    The new methods could call this (private) method.



core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java
Lines 521 (patched)
<https://reviews.apache.org/r/66656/#comment299314>

    Please add assert message



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 1 (patched)
<https://reviews.apache.org/r/66656/#comment299305>

    Please add licence information to avoid RAT warning



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 251 (patched)
<https://reviews.apache.org/r/66656/#comment299317>

    please insert line break to follow the Oozie coding style



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 252 (patched)
<https://reviews.apache.org/r/66656/#comment299318>

    Please add assert message



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 253 (patched)
<https://reviews.apache.org/r/66656/#comment299319>

    Please add assert message



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 280 (patched)
<https://reviews.apache.org/r/66656/#comment299321>

    If we need to add three comment lines because this test has 3 important subcases, then we might separate it into 3 methods.



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 357 (patched)
<https://reviews.apache.org/r/66656/#comment299322>

    Please avoid javadoc warning



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 432 (patched)
<https://reviews.apache.org/r/66656/#comment299323>

    Please add linebreak



core/src/test/java/org/apache/oozie/action/hadoop/TestShareLibExcluder.java
Lines 42 (patched)
<https://reviews.apache.org/r/66656/#comment299324>

    typo: EMPTY_ARRAY



core/src/test/java/org/apache/oozie/action/hadoop/TestShareLibExcluder.java
Lines 54 (patched)
<https://reviews.apache.org/r/66656/#comment299325>

    It's a constant, can we use upper case letters.
    
    Can we use a format more similar to the sharelib name used by Oozie?


- Andras Salamon


On March 1, 2019, 2:44 p.m., Mate Juhasz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66656/
> -----------------------------------------------------------
> 
> (Updated March 1, 2019, 2:44 p.m.)
> 
> 
> Review request for oozie, András Piros and Denes Bodo.
> 
> 
> Bugs: OOZIE-1624
>     https://issues.apache.org/jira/browse/OOZIE-1624
> 
> 
> Repository: oozie-git
> 
> 
> Description
> -------
> 
> OOZIE-1624 Exclusion pattern for sharelib.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 231b38ea 
>   core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java PRE-CREATION 
>   core/src/main/java/org/apache/oozie/service/ShareLibService.java b88dab3a 
>   core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 05511e4c 
>   core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 6383e814 
>   core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java PRE-CREATION 
>   core/src/test/java/org/apache/oozie/action/hadoop/TestShareLibExcluder.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/66656/diff/6/
> 
> 
> Testing
> -------
> 
> Tested on a cluster
> 
> 
> Thanks,
> 
> Mate Juhasz
> 
>

Re: Review Request 66656: Exclusion pattern for sharelib.

Posted by Denes Bodo <bo...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66656/#review213472
-----------------------------------------------------------



Please use final modifier where possible in the new code.


core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 1164-1165 (patched)
<https://reviews.apache.org/r/66656/#comment299404>

    In case of an error wi shall throw an exception. It this is not a real error, then I think we should use WARN level.



core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 1173 (patched)
<https://reviews.apache.org/r/66656/#comment299405>

    Are you sure it is not WARN?



core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java
Lines 1177 (patched)
<https://reviews.apache.org/r/66656/#comment299406>

    Are you sure it is not WARN?



core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java
Lines 46 (patched)
<https://reviews.apache.org/r/66656/#comment299407>

    ... pattern is not ...



core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java
Lines 62 (patched)
<https://reviews.apache.org/r/66656/#comment299411>

    maybeExcludePattern can be final



core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java
Lines 64 (patched)
<https://reviews.apache.org/r/66656/#comment299410>

    excludePattern can be final



core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java
Lines 74 (patched)
<https://reviews.apache.org/r/66656/#comment299408>

    please be consistent with *final* modifier in parameter list. Use it everywhere or use nowhere.



core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java
Lines 86 (patched)
<https://reviews.apache.org/r/66656/#comment299409>

    What does "skipping" mean? I would like to see a bit more detailed message what hapens this case.



core/src/main/java/org/apache/oozie/service/ShareLibService.java
Lines 624 (patched)
<https://reviews.apache.org/r/66656/#comment299412>

    What can cause that IOException?



core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java
Lines 431 (patched)
<https://reviews.apache.org/r/66656/#comment299403>

    please remove trailing whitespaces


- Denes Bodo


On March 5, 2019, 4:54 p.m., Mate Juhasz wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66656/
> -----------------------------------------------------------
> 
> (Updated March 5, 2019, 4:54 p.m.)
> 
> 
> Review request for oozie, András Piros and Denes Bodo.
> 
> 
> Bugs: OOZIE-1624
>     https://issues.apache.org/jira/browse/OOZIE-1624
> 
> 
> Repository: oozie-git
> 
> 
> Description
> -------
> 
> OOZIE-1624 Exclusion pattern for sharelib.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/oozie/action/hadoop/JavaActionExecutor.java 231b38ea 
>   core/src/main/java/org/apache/oozie/action/hadoop/ShareLibExcluder.java PRE-CREATION 
>   core/src/main/java/org/apache/oozie/service/ShareLibService.java b88dab3a 
>   core/src/test/java/org/apache/oozie/action/hadoop/ActionExecutorTestCase.java 05511e4c 
>   core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutor.java 6383e814 
>   core/src/test/java/org/apache/oozie/action/hadoop/TestJavaActionExecutorLibAddition.java PRE-CREATION 
>   core/src/test/java/org/apache/oozie/action/hadoop/TestShareLibExcluder.java PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/66656/diff/7/
> 
> 
> Testing
> -------
> 
> Tested on a cluster
> 
> 
> Thanks,
> 
> Mate Juhasz
> 
>