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 14:01:35 UTC
Re: Review Request 66930: OOZIE-2791 ShareLib installation may fail
on busy Hadoop clusters
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66930/#review205638
-----------------------------------------------------------
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Line 226 (original), 245 (patched)
<https://reviews.apache.org/r/66930/#comment288529>
Does it help to have a reference to the thread pool in every instance? We just have a single executor. I'd rather pass the executor in the methods than having this here, especially because it's not used by the class itself.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 256-259 (patched)
<https://reviews.apache.org/r/66930/#comment288528>
To me this looks weird. The class is named "configuration", which is telling me that it holds data. But it doesn't just hold data, it also performs an FS operation.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 339 (patched)
<https://reviews.apache.org/r/66930/#comment288523>
Extract magic number to a static finas with good naming
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 350-351 (patched)
<https://reviews.apache.org/r/66930/#comment288524>
Extract magic numbers to static finals with good naming
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 360-362 (patched)
<https://reviews.apache.org/r/66930/#comment288525>
Add error message to checkArgument()
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Line 240 (original), 405 (patched)
<https://reviews.apache.org/r/66930/#comment288527>
If we're not interested in the actual contents of the exception, then using a boolean to indicate a failure is a better choice.
tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java
Lines 141 (patched)
<https://reviews.apache.org/r/66930/#comment288531>
This should be in a finally block
- Peter Bacsko
On jún. 18, 2018, 10:03 de, Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66930/
> -----------------------------------------------------------
>
> (Updated jún. 18, 2018, 10:03 de)
>
>
> Review request for oozie, András Piros and Peter Cseh.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> On a busy Hadoop cluster it can happen that users cannot install properly
> Oozie ShareLib.
>
>
> Diffs
> -----
>
> tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java dce1c559
> tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java PRE-CREATION
> tools/src/test/java/org/apache/oozie/tools/TestBlockSizeCalculator.java PRE-CREATION
> tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java PRE-CREATION
> tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java PRE-CREATION
> tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java ccad273b
>
>
> Diff: https://reviews.apache.org/r/66930/diff/6/
>
>
> Testing
> -------
>
> Tested manually
>
>
> Thanks,
>
> Kinga Marton
>
>
Re: Review Request 66930: OOZIE-2791 ShareLib installation may fail
on busy Hadoop clusters
Posted by Kinga Marton via Review Board <no...@reviews.apache.org>.
> On July 2, 2018, 2:01 p.m., Peter Bacsko wrote:
> > tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
> > Lines 256-259 (patched)
> > <https://reviews.apache.org/r/66930/diff/6/?file=2041662#file2041662line257>
> >
> > To me this looks weird. The class is named "configuration", which is telling me that it holds data. But it doesn't just hold data, it also performs an FS operation.
You are right, I have moved it to ConcurrentCopyFromLocal
- Kinga
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66930/#review205638
-----------------------------------------------------------
On June 18, 2018, 10:03 a.m., Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66930/
> -----------------------------------------------------------
>
> (Updated June 18, 2018, 10:03 a.m.)
>
>
> Review request for oozie, András Piros and Peter Cseh.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> On a busy Hadoop cluster it can happen that users cannot install properly
> Oozie ShareLib.
>
>
> Diffs
> -----
>
> tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java dce1c559
> tools/src/test/java/org/apache/oozie/tools/OozieSharelibFileOperations.java PRE-CREATION
> tools/src/test/java/org/apache/oozie/tools/TestBlockSizeCalculator.java PRE-CREATION
> tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java PRE-CREATION
> tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java PRE-CREATION
> tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java ccad273b
>
>
> Diff: https://reviews.apache.org/r/66930/diff/6/
>
>
> Testing
> -------
>
> Tested manually
>
>
> Thanks,
>
> Kinga Marton
>
>