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/06/04 13:55:18 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/
-----------------------------------------------------------
(Updated June 4, 2018, 1:55 p.m.)
Review request for oozie, András Piros and Peter Cseh.
Changes
-------
review fix + test classes for CopyTaskCallable and ConcurrentCopyFromLocal
Repository: oozie-git
Description
-------
On a busy Hadoop cluster it can happen that users cannot install properly
Oozie ShareLib.
Diffs (updated)
-----
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java dce1c559
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/3/
Changes: https://reviews.apache.org/r/66930/diff/2-3/
Testing
-------
Tested manually
Thanks,
Kinga Marton
Re: Review Request 66930: OOZIE-2791 ShareLib installation may fail
on busy Hadoop clusters
Posted by Peter Bacsko via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66930/#review204598
-----------------------------------------------------------
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 70-71 (patched)
<https://reviews.apache.org/r/66930/#comment287176>
Please check if these constants can be imported from some Hadoop class.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Line 184 (original), 193-194 (patched)
<https://reviews.apache.org/r/66930/#comment287177>
Does getConf() return a Configuration object? If so, call getLong() so you don't have to call parseLong().
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 214 (patched)
<https://reviews.apache.org/r/66930/#comment287178>
What about using logError() ?
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 294 (patched)
<https://reviews.apache.org/r/66930/#comment287179>
Nit: unnecessary white space after "0"
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 298 (patched)
<https://reviews.apache.org/r/66930/#comment287180>
Ok, I know this is nitpicking, but still... :D It's better to write "(ratio + 1)", so with whitespaces.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Line 268 (original), 439 (patched)
<https://reviews.apache.org/r/66930/#comment287181>
Initially we did something similar in the DB retry logic.
The problem with this approach is that retryDelayInMs can grow to a large number in an unfortunate situation. We're better off cap it to a reasonable maximum like 30 secs or something.
tools/src/test/java/org/apache/oozie/tools/TestBlockSizeCalculator.java
Lines 40 (patched)
<https://reviews.apache.org/r/66930/#comment287182>
Nite: could you move this lone comma to the end of the previous line?
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIHelper.java
Lines 35 (patched)
<https://reviews.apache.org/r/66930/#comment287183>
Does the methods in this class have to be protected? If this is an utility class and not intended to be subclassed, we should be fine with public methods (plus declare it final). Also, if we don't want instances, we can make the constructor private.
- Peter Bacsko
On jún. 6, 2018, 12:34 du, Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66930/
> -----------------------------------------------------------
>
> (Updated jún. 6, 2018, 12:34 du)
>
>
> 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/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
> tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIHelper.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/66930/diff/5/
>
>
> 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
>
>
Re: Review Request 66930: OOZIE-2791 ShareLib installation may fail
on busy Hadoop clusters
Posted by Peter Bacsko via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
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 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/66930/#review204922
-----------------------------------------------------------
Ship it!
Ship It!
- András Piros
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
>
>
Re: Review Request 66930: OOZIE-2791 ShareLib installation may fail
on busy Hadoop clusters
Posted by Peter Bacsko via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66930/#review205714
-----------------------------------------------------------
Ship it!
Ship It!
- Peter Bacsko
On júl. 3, 2018, 3:24 du, Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66930/
> -----------------------------------------------------------
>
> (Updated júl. 3, 2018, 3:24 du)
>
>
> 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 f53d987c
>
>
> Diff: https://reviews.apache.org/r/66930/diff/8/
>
>
> 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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66930/
-----------------------------------------------------------
(Updated July 3, 2018, 3:24 p.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 (updated)
-----
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 f53d987c
Diff: https://reviews.apache.org/r/66930/diff/8/
Changes: https://reviews.apache.org/r/66930/diff/7-8/
Testing
-------
Tested manually
Thanks,
Kinga Marton
Re: Review Request 66930: OOZIE-2791 ShareLib installation may fail
on busy Hadoop clusters
Posted by Peter Bacsko via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66930/#review205672
-----------------------------------------------------------
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Line 226 (original), 245 (patched)
<https://reviews.apache.org/r/66930/#comment288564>
Remove comment
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 251 (patched)
<https://reviews.apache.org/r/66930/#comment288565>
Remove comment
tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java
Lines 129 (patched)
<https://reviews.apache.org/r/66930/#comment288566>
Almost - I think the try block should include more code, just to be safe. At least from the for loop where you call listFiles(). At that point, the pool already exists.
- Peter Bacsko
On júl. 3, 2018, 8:12 de, Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66930/
> -----------------------------------------------------------
>
> (Updated júl. 3, 2018, 8:12 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 f53d987c
>
>
> Diff: https://reviews.apache.org/r/66930/diff/7/
>
>
> 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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66930/
-----------------------------------------------------------
(Updated July 3, 2018, 8:12 a.m.)
Review request for oozie, András Piros and Peter Cseh.
Changes
-------
review fix
Repository: oozie-git
Description
-------
On a busy Hadoop cluster it can happen that users cannot install properly
Oozie ShareLib.
Diffs (updated)
-----
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 f53d987c
Diff: https://reviews.apache.org/r/66930/diff/7/
Changes: https://reviews.apache.org/r/66930/diff/6-7/
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>.
-----------------------------------------------------------
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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/66930/diff/5-6/
Testing
-------
Tested manually
Thanks,
Kinga Marton
Re: Review Request 66930: OOZIE-2791 ShareLib installation may fail
on busy Hadoop clusters
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/66930/#review204384
-----------------------------------------------------------
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 337-341 (patched)
<https://reviews.apache.org/r/66930/#comment286871>
Extract parameters to local variables for better readability.
tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java
Lines 35 (patched)
<https://reviews.apache.org/r/66930/#comment286877>
When we have `JUnit4Runner` (have we?) we can use `@Rule public TemporaryFolder tmpFolder;` instead.
tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java
Lines 43 (patched)
<https://reviews.apache.org/r/66930/#comment286875>
Best is to inject necessary, stateful components into `TestOozieSharelibCLIHelper` (need to find a better name...) constructor.
tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java
Lines 51 (patched)
<https://reviews.apache.org/r/66930/#comment286874>
Best is to inject necessary, stateful components into `TestOozieSharelibCLIHelper` (need to find a better name...) constructor, and call e.g. `services.destroy()` explicitly here.
tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java
Lines 55 (patched)
<https://reviews.apache.org/r/66930/#comment286876>
I don't get from the unit test name alone what it's supposed to test and what is the expectation.
tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java
Lines 42 (patched)
<https://reviews.apache.org/r/66930/#comment286879>
When we have `JUnit4Runner` (have we?) we can use `@Rule public TemporaryFolder tmpFolder;` instead.
tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java
Lines 44 (patched)
<https://reviews.apache.org/r/66930/#comment286882>
Best is to inject necessary, stateful components into `TestOozieSharelibCLIHelper` (need to find a better name...) constructor.
tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java
Lines 57 (patched)
<https://reviews.apache.org/r/66930/#comment286884>
Best is to inject necessary, stateful components into `TestOozieSharelibCLIHelper` (need to find a better name...) constructor, and call e.g. `services.destroy()` explicitly here.
tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java
Lines 61 (patched)
<https://reviews.apache.org/r/66930/#comment286878>
Same as above - please provide a more describing name.
It would be great to have different unit test cases with and without an own `ExecutorService`, testing with a single thread / many concurrent threads, and covering the previously failing functionality as well.
tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java
Lines 64-65 (patched)
<https://reviews.apache.org/r/66930/#comment286881>
Copying more files per thread can also be a different test case.
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Line 51 (original), 39 (patched)
<https://reviews.apache.org/r/66930/#comment286880>
When we have `JUnit4Runner` (have we?) we can use `@Rule public TemporaryFolder tmpFolder;` instead.
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Lines 41 (patched)
<https://reviews.apache.org/r/66930/#comment286883>
Best is to inject necessary, stateful components into `TestOozieSharelibCLIHelper` (need to find a better name...) constructor.
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Lines 58 (patched)
<https://reviews.apache.org/r/66930/#comment286885>
Best is to inject necessary, stateful components into `TestOozieSharelibCLIHelper` (need to find a better name...) constructor, and call e.g. `services.destroy()` explicitly here.
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIHelper.java
Lines 35 (patched)
<https://reviews.apache.org/r/66930/#comment286870>
`OozieSharelibFileOperations` is a better name. It's not a test class, and I don't like the words `Helper`, `Util`, and the like as class or method names.
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIHelper.java
Lines 38-40 (patched)
<https://reviews.apache.org/r/66930/#comment286869>
Would have all these as `private final`, and injected via constructor.
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIHelper.java
Lines 81-90 (patched)
<https://reviews.apache.org/r/66930/#comment286867>
Would rather inject a `FileSystem` instance from the outside and `init()` it from the caller class' `setUp()` method.
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIHelper.java
Lines 92-103 (patched)
<https://reviews.apache.org/r/66930/#comment286866>
Would rather inject a `Services` instance from the outside and `init()` it from the caller class' `setUp()` method.
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIHelper.java
Lines 105-109 (patched)
<https://reviews.apache.org/r/66930/#comment286865>
Would rather inject a `Services` instance from the outside and `destroy()` it from the caller class' `tearDown()` method.
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIHelper.java
Lines 111-117 (patched)
<https://reviews.apache.org/r/66930/#comment286868>
Would rather inject `systemLibPath` from the outside.
- András Piros
On June 6, 2018, 12:34 p.m., Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66930/
> -----------------------------------------------------------
>
> (Updated June 6, 2018, 12:34 p.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/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
> tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIHelper.java PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/66930/diff/5/
>
>
> 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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66930/
-----------------------------------------------------------
(Updated June 6, 2018, 12:34 p.m.)
Review request for oozie, András Piros and Peter Cseh.
Changes
-------
review fix
Repository: oozie-git
Description
-------
On a busy Hadoop cluster it can happen that users cannot install properly
Oozie ShareLib.
Diffs (updated)
-----
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java dce1c559
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
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLIHelper.java PRE-CREATION
Diff: https://reviews.apache.org/r/66930/diff/5/
Changes: https://reviews.apache.org/r/66930/diff/4-5/
Testing
-------
Tested manually
Thanks,
Kinga Marton
Re: Review Request 66930: OOZIE-2791 ShareLib installation may fail
on busy Hadoop clusters
Posted by Peter Cseh via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66930/#review204339
-----------------------------------------------------------
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 252 (patched)
<https://reviews.apache.org/r/66930/#comment286780>
Why is a delete call here? Is it to clean up for retries in case the previous attempt created the file but failed during upload?
- Peter Cseh
On June 5, 2018, 8:37 a.m., Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66930/
> -----------------------------------------------------------
>
> (Updated June 5, 2018, 8:37 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/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/4/
>
>
> Testing
> -------
>
> Tested manually
>
>
> Thanks,
>
> Kinga Marton
>
>
Re: Review Request 66930: OOZIE-2791 ShareLib installation may fail
on busy Hadoop clusters
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/66930/#review204314
-----------------------------------------------------------
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 194 (patched)
<https://reviews.apache.org/r/66930/#comment286740>
Would handle `NumberFormatException` separately.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Line 223 (original), 238 (patched)
<https://reviews.apache.org/r/66930/#comment286742>
Can it be `private`, and if not, is it `@VisibleForTesting`?
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 257 (patched)
<https://reviews.apache.org/r/66930/#comment286736>
Do we surely need to override `Object#equals()`?
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 267-272 (patched)
<https://reviews.apache.org/r/66930/#comment286738>
`fs` and `threadPool` should not be part of `equals()`.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 280 (patched)
<https://reviews.apache.org/r/66930/#comment286737>
Do we surely need to override `Object#hashCode()`?
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 281-282 (patched)
<https://reviews.apache.org/r/66930/#comment286739>
`fs` and `threadPool` should not be part of `hashCode()`.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 290 (patched)
<https://reviews.apache.org/r/66930/#comment286741>
Can it be `private`, and if not, is it `@VisibleForTesting`?
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 305 (patched)
<https://reviews.apache.org/r/66930/#comment286743>
Can it be `private`, and if not, is it `@VisibleForTesting`?
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Line 249 (original), 417 (patched)
<https://reviews.apache.org/r/66930/#comment286744>
Why use `String#format()` here?
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Line 252 (original), 420-422 (patched)
<https://reviews.apache.org/r/66930/#comment286745>
This should be in `finally`.
tools/src/test/java/org/apache/oozie/tools/TestBlockSizeCalculator.java
Lines 24 (patched)
<https://reviews.apache.org/r/66930/#comment286746>
I like this test case :)
tools/src/test/java/org/apache/oozie/tools/TestConcurrentCopyFromLocal.java
Lines 31 (patched)
<https://reviews.apache.org/r/66930/#comment286747>
Would rather not extend `TestOozieSharelibCLI` (because don't want to rerun all those tests once more), but extract the functionality needed from `TestOozieSharelibCLI` and would use rather composition over inheritance in both cases.
tools/src/test/java/org/apache/oozie/tools/TestCopyTaskCallable.java
Lines 38 (patched)
<https://reviews.apache.org/r/66930/#comment286748>
Would rather not extend `TestOozieSharelibCLI` (because don't want to rerun all those tests once more), but extract the functionality needed from `TestOozieSharelibCLI` and would use rather composition over inheritance in both cases.
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Line 221 (original), 230-236 (patched)
<https://reviews.apache.org/r/66930/#comment286749>
Would rather extract the functionality needed from `TestOozieSharelibCLI` and would use the extracted class by composition over inheritance in every case.
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Line 240 (original), 255 (patched)
<https://reviews.apache.org/r/66930/#comment286750>
Would rather extract the functionality needed from `TestOozieSharelibCLI` and would use the extracted class by composition over inheritance in every case.
tools/src/test/java/org/apache/oozie/tools/TestOozieSharelibCLI.java
Line 244 (original), 259 (patched)
<https://reviews.apache.org/r/66930/#comment286751>
Would rather extract the functionality needed from `TestOozieSharelibCLI` and would use the extracted class by composition over inheritance in every case.
- András Piros
On June 5, 2018, 8:37 a.m., Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66930/
> -----------------------------------------------------------
>
> (Updated June 5, 2018, 8:37 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/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/4/
>
>
> Testing
> -------
>
> Tested manually
>
>
> Thanks,
>
> Kinga Marton
>
>
Re: Review Request 66930: OOZIE-2791 ShareLib installation may fail
on busy Hadoop clusters
Posted by Peter Cseh via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66930/#review204336
-----------------------------------------------------------
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Line 254 (original), 424 (patched)
<https://reviews.apache.org/r/66930/#comment286777>
We could check for failedCopyTasks.isEmpty() here, can't we?
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 429 (patched)
<https://reviews.apache.org/r/66930/#comment286776>
the "t" in the message might be different here than the original "t" that was null-checked. Please use an error from the failed task list.
- Peter Cseh
On June 5, 2018, 8:37 a.m., Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66930/
> -----------------------------------------------------------
>
> (Updated June 5, 2018, 8:37 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/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/4/
>
>
> 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>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66930/
-----------------------------------------------------------
(Updated June 5, 2018, 8:37 a.m.)
Review request for oozie, András Piros and Peter Cseh.
Changes
-------
findbug fix
Repository: oozie-git
Description
-------
On a busy Hadoop cluster it can happen that users cannot install properly
Oozie ShareLib.
Diffs (updated)
-----
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java dce1c559
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/4/
Changes: https://reviews.apache.org/r/66930/diff/3-4/
Testing
-------
Tested manually
Thanks,
Kinga Marton