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/05/03 10:07:18 UTC
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/
-----------------------------------------------------------
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 dce1c559b
Diff: https://reviews.apache.org/r/66930/diff/1/
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 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 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
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 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 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/#review202830
-----------------------------------------------------------
tools/src/test/java/org/apache/oozie/tools/TestBlockSizeCalculator.java
Lines 32 (patched)
<https://reviews.apache.org/r/66930/#comment284822>
Add assertion message.
tools/src/test/java/org/apache/oozie/tools/TestBlockSizeCalculator.java
Lines 39 (patched)
<https://reviews.apache.org/r/66930/#comment284823>
Add assertion message.
tools/src/test/java/org/apache/oozie/tools/TestBlockSizeCalculator.java
Lines 45 (patched)
<https://reviews.apache.org/r/66930/#comment284824>
Add assertion message.
- András Piros
On May 10, 2018, 7:59 a.m., Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66930/
> -----------------------------------------------------------
>
> (Updated May 10, 2018, 7:59 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
>
>
> Diff: https://reviews.apache.org/r/66930/diff/2/
>
>
> 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/#review202829
-----------------------------------------------------------
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 297 (patched)
<https://reviews.apache.org/r/66930/#comment284820>
A separate unit test class is needed, `TestCopyTaskCallable`, now that we wrote this to be testable.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 310-316 (patched)
<https://reviews.apache.org/r/66930/#comment284814>
Just before setting any fields, `Preconditions#checkNotNull()` wouldn't hurt.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 334 (patched)
<https://reviews.apache.org/r/66930/#comment284821>
A separate unit test class is needed, `TestConcurrentCopyFromLocal`, now that we wrote this to be testable.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 346-349 (patched)
<https://reviews.apache.org/r/66930/#comment284815>
Just before setting any fields, `Preconditions#checkArgument()` wouldn't hurt.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 424-425 (patched)
<https://reviews.apache.org/r/66930/#comment284816>
I'd have a method inside `CopyTaskConfiguration` to perform that stuff.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 429 (patched)
<https://reviews.apache.org/r/66930/#comment284817>
We should also log something saying we didn't manage to copy a file (exact source / destination paths, and error message provided) but we still proceed.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 437 (patched)
<https://reviews.apache.org/r/66930/#comment284818>
We need to express why we fail.
tools/src/test/java/org/apache/oozie/tools/TestBlockSizeCalculator.java
Lines 24 (patched)
<https://reviews.apache.org/r/66930/#comment284819>
Big thumbs up :)
- András Piros
On May 10, 2018, 7:59 a.m., Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66930/
> -----------------------------------------------------------
>
> (Updated May 10, 2018, 7:59 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
>
>
> Diff: https://reviews.apache.org/r/66930/diff/2/
>
>
> 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 May 10, 2018, 7:59 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/TestBlockSizeCalculator.java PRE-CREATION
Diff: https://reviews.apache.org/r/66930/diff/2/
Changes: https://reviews.apache.org/r/66930/diff/1-2/
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 May 3, 2018, 3:33 p.m., András Piros wrote:
> > tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
> > Lines 224-235 (patched)
> > <https://reviews.apache.org/r/66930/diff/1/?file=2016229#file2016229line224>
> >
> > Please extract to another `static final class`, and add integration tests that use `XTestCase` to check copying real data onto `MiniDFSCluster`.
TestOozieSharelibCLI:testOozieSharelibCLICreateConcurrent() is testing if the files are copied in concurrent mode.
> On May 3, 2018, 3:33 p.m., András Piros wrote:
> > tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
> > Lines 322 (patched)
> > <https://reviews.apache.org/r/66930/diff/1/?file=2016229#file2016229line323>
> >
> > I'm just wondering whether in case any errors we should 1) rollback the stuff we halfway managed to copy 2) die after this `System.err.println()` call.
1. The possibility of a "partial sharelib update" is handled with OOZIE-2629.
2. In case of an Exception the exit status will be 1. This is handled at the end of the run() method.
> On May 3, 2018, 3:33 p.m., András Piros wrote:
> > tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
> > Lines 349-351 (patched)
> > <https://reviews.apache.org/r/66930/diff/1/?file=2016229#file2016229line350>
> >
> > I'm just wondering whether in case any errors we should 1) rollback the stuff we halfway managed to copy 2) die after this `System.err.println()` call.
1. The possibility of a "partial sharelib update" is handled with OOZIE-2629.
2. In case of an Exception the exit status will be 1. This is handled at the end of the run() method.
- Kinga
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66930/#review202354
-----------------------------------------------------------
On May 10, 2018, 7:59 a.m., Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66930/
> -----------------------------------------------------------
>
> (Updated May 10, 2018, 7:59 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
>
>
> Diff: https://reviews.apache.org/r/66930/diff/2/
>
>
> 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/#review202354
-----------------------------------------------------------
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 224-235 (patched)
<https://reviews.apache.org/r/66930/#comment284133>
Please extract to another `static final class`, and add integration tests that use `XTestCase` to check copying real data onto `MiniDFSCluster`.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 226 (patched)
<https://reviews.apache.org/r/66930/#comment284125>
Would be way nicer to have this `ExecutorService` as `private final` field.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 233 (patched)
<https://reviews.apache.org/r/66930/#comment284126>
A log message to note when all the tasks are done would be nice.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 245-260 (patched)
<https://reviews.apache.org/r/66930/#comment284130>
Please extract this `Callable` to another `private static final class ... extends Callable<CopyTaskConfiguration>`
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 252-254 (patched)
<https://reviews.apache.org/r/66930/#comment284131>
Please extract parameters to well-named local variables.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 266-276 (patched)
<https://reviews.apache.org/r/66930/#comment284132>
Please extract to `static class BlockSizeCalculator`, and add unit tests covering its functionality.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 303 (patched)
<https://reviews.apache.org/r/66930/#comment284134>
`System.err.println(e.getMessage())` instead.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 307 (patched)
<https://reviews.apache.org/r/66930/#comment284135>
`System.err.println("Retrying to copy ...")`
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 322 (patched)
<https://reviews.apache.org/r/66930/#comment284136>
I'm just wondering whether in case any errors we should 1) rollback the stuff we halfway managed to copy 2) die after this `System.err.println()` call.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 349-351 (patched)
<https://reviews.apache.org/r/66930/#comment284137>
I'm just wondering whether in case any errors we should 1) rollback the stuff we halfway managed to copy 2) die after this `System.err.println()` call.
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Line 259 (original), 355 (patched)
<https://reviews.apache.org/r/66930/#comment284127>
`private final static class`
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 260-262 (original), 356-359 (patched)
<https://reviews.apache.org/r/66930/#comment284128>
`private final`
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Line 264 (original), 361 (patched)
<https://reviews.apache.org/r/66930/#comment284129>
`private`
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java
Lines 370-377 (patched)
<https://reviews.apache.org/r/66930/#comment284138>
Please add curly braces to this autogenerated method.
- András Piros
On May 3, 2018, 10:07 a.m., Kinga Marton wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66930/
> -----------------------------------------------------------
>
> (Updated May 3, 2018, 10:07 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 dce1c559b
>
>
> Diff: https://reviews.apache.org/r/66930/diff/1/
>
>
> Testing
> -------
>
> Tested manually
>
>
> Thanks,
>
> Kinga Marton
>
>