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 <pb...@cloudera.com> on 2017/09/15 12:07:49 UTC
Review Request 62350: OOZIE-3054 Disable erasure coding for sharelib
if Oozie runs on Hadoop 3
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62350/
-----------------------------------------------------------
Review request for oozie, András Piros, Peter Cseh, and Robert Kanter.
Repository: oozie-git
Description
-------
See https://issues.apache.org/jira/browse/OOZIE-3054
Diffs
-----
tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java PRE-CREATION
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java a844aa0f8
Diff: https://reviews.apache.org/r/62350/diff/1/
Testing
-------
Thanks,
Peter Bacsko
Re: Review Request 62350: OOZIE-3054 Disable erasure coding for
sharelib if Oozie runs on Hadoop 3
Posted by Peter Bacsko <pb...@cloudera.com>.
> On szept. 15, 2017, 6:51 du, Robert Kanter wrote:
> > tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java
> > Lines 42 (patched)
> > <https://reviews.apache.org/r/62350/diff/1/?file=1827863#file1827863line42>
> >
> > It would be good to have a unit test for this; but if that's not practical, I'm okay with you just verifying this on a cluster.
Unfortunately it isn't practical. To test the code path that disables EC, we need the Hadoop 3 classes on the classpath or generate these classes with matching method signatures dynamically with a tool like Javassist, but that would be too much trouble.
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62350/#review185504
-----------------------------------------------------------
On szept. 15, 2017, 12:07 du, Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62350/
> -----------------------------------------------------------
>
> (Updated szept. 15, 2017, 12:07 du)
>
>
> Review request for oozie, András Piros, Peter Cseh, and Robert Kanter.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> See https://issues.apache.org/jira/browse/OOZIE-3054
>
>
> Diffs
> -----
>
> tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java PRE-CREATION
> tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java a844aa0f8
>
>
> Diff: https://reviews.apache.org/r/62350/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Peter Bacsko
>
>
Re: Review Request 62350: OOZIE-3054 Disable erasure coding for
sharelib if Oozie runs on Hadoop 3
Posted by Robert Kanter <rk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62350/#review185504
-----------------------------------------------------------
tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java
Lines 42 (patched)
<https://reviews.apache.org/r/62350/#comment261809>
It would be good to have a unit test for this; but if that's not practical, I'm okay with you just verifying this on a cluster.
- Robert Kanter
On Sept. 15, 2017, 12:07 p.m., Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62350/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2017, 12:07 p.m.)
>
>
> Review request for oozie, András Piros, Peter Cseh, and Robert Kanter.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> See https://issues.apache.org/jira/browse/OOZIE-3054
>
>
> Diffs
> -----
>
> tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java PRE-CREATION
> tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java a844aa0f8
>
>
> Diff: https://reviews.apache.org/r/62350/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Peter Bacsko
>
>
Re: Review Request 62350: OOZIE-3054 Disable erasure coding for
sharelib if Oozie runs on Hadoop 3
Posted by Peter Bacsko <pb...@cloudera.com>.
> On szept. 15, 2017, 12:27 du, András Piros wrote:
> > tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java
> > Lines 68 (patched)
> > <https://reviews.apache.org/r/62350/diff/1/?file=1827863#file1827863line68>
> >
> > Since `Class.forName()` is an expensive operation, it might be worth caching its result.
We do this very rarely, only when we copy the sharelib, so it's OK in this case.
> On szept. 15, 2017, 12:27 du, András Piros wrote:
> > tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java
> > Lines 80 (patched)
> > <https://reviews.apache.org/r/62350/diff/1/?file=1827863#file1827863line80>
> >
> > Would be better name all the possible `Exception` subclasses that may arise.
There are too many. All of them are related to dynamic method access/invocation. I wouldn't bother naming them separately.
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62350/#review185480
-----------------------------------------------------------
On szept. 15, 2017, 12:07 du, Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62350/
> -----------------------------------------------------------
>
> (Updated szept. 15, 2017, 12:07 du)
>
>
> Review request for oozie, András Piros, Peter Cseh, and Robert Kanter.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> See https://issues.apache.org/jira/browse/OOZIE-3054
>
>
> Diffs
> -----
>
> tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java PRE-CREATION
> tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java a844aa0f8
>
>
> Diff: https://reviews.apache.org/r/62350/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Peter Bacsko
>
>
Re: Review Request 62350: OOZIE-3054 Disable erasure coding for
sharelib if Oozie runs on Hadoop 3
Posted by Peter Bacsko <pb...@cloudera.com>.
> On szept. 15, 2017, 12:27 du, András Piros wrote:
> > Some comments. Please also add unit tests.
A bit too difficult to add unit tests to verify this modification. See my explanation to Robert.
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62350/#review185480
-----------------------------------------------------------
On szept. 15, 2017, 12:07 du, Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62350/
> -----------------------------------------------------------
>
> (Updated szept. 15, 2017, 12:07 du)
>
>
> Review request for oozie, András Piros, Peter Cseh, and Robert Kanter.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> See https://issues.apache.org/jira/browse/OOZIE-3054
>
>
> Diffs
> -----
>
> tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java PRE-CREATION
> tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java a844aa0f8
>
>
> Diff: https://reviews.apache.org/r/62350/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Peter Bacsko
>
>
Re: Review Request 62350: OOZIE-3054 Disable erasure coding for
sharelib if Oozie runs on Hadoop 3
Posted by András Piros <an...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62350/#review185480
-----------------------------------------------------------
Some comments. Please also add unit tests.
tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java
Lines 50 (patched)
<https://reviews.apache.org/r/62350/#comment261778>
We should have log messages for the `if` case when we want to set, and for the now-unsettled `else` case when we don't want to set.
tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java
Lines 68 (patched)
<https://reviews.apache.org/r/62350/#comment261772>
Since `Class.forName()` is an expensive operation, it might be worth caching its result.
tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java
Lines 75 (patched)
<https://reviews.apache.org/r/62350/#comment261775>
Rename to `getReplicationPolicy()`.
tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java
Lines 80 (patched)
<https://reviews.apache.org/r/62350/#comment261773>
Would be better name all the possible `Exception` subclasses that may arise.
tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java
Lines 82 (patched)
<https://reviews.apache.org/r/62350/#comment261774>
Since we don't throw `RuntimeException` within `isHadoop3()`, and the first part of this method is covered there, I suggest having a common method `getECPoliciesClass()` that throws `ClassNotFoundException` and is used by both `isHadoop3()` and `getReplicationPolicy()`.
tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java
Lines 103 (patched)
<https://reviews.apache.org/r/62350/#comment261776>
`System.err.println()`
tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java
Lines 112 (patched)
<https://reviews.apache.org/r/62350/#comment261777>
`System.err.println()`
- András Piros
On Sept. 15, 2017, 12:07 p.m., Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62350/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2017, 12:07 p.m.)
>
>
> Review request for oozie, András Piros, Peter Cseh, and Robert Kanter.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> See https://issues.apache.org/jira/browse/OOZIE-3054
>
>
> Diffs
> -----
>
> tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java PRE-CREATION
> tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java a844aa0f8
>
>
> Diff: https://reviews.apache.org/r/62350/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Peter Bacsko
>
>
Re: Review Request 62350: OOZIE-3054 Disable erasure coding for
sharelib if Oozie runs on Hadoop 3
Posted by András Piros <an...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62350/#review185788
-----------------------------------------------------------
Ship it!
Ship It!
- András Piros
On Sept. 19, 2017, 9:52 a.m., Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62350/
> -----------------------------------------------------------
>
> (Updated Sept. 19, 2017, 9:52 a.m.)
>
>
> Review request for oozie, András Piros, Peter Cseh, and Robert Kanter.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> See https://issues.apache.org/jira/browse/OOZIE-3054
>
>
> Diffs
> -----
>
> tools/src/main/java/org/apache/oozie/tools/ECPolicyDisabler.java PRE-CREATION
> tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java a844aa0f8
>
>
> Diff: https://reviews.apache.org/r/62350/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Peter Bacsko
>
>
Re: Review Request 62350: OOZIE-3054 Disable erasure coding for
sharelib if Oozie runs on Hadoop 3
Posted by Peter Bacsko <pb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62350/
-----------------------------------------------------------
(Updated szept. 19, 2017, 9:52 de)
Review request for oozie, András Piros, Peter Cseh, and Robert Kanter.
Changes
-------
Addressing review comments
Repository: oozie-git
Description
-------
See https://issues.apache.org/jira/browse/OOZIE-3054
Diffs (updated)
-----
tools/src/main/java/org/apache/oozie/tools/ECPolicyDisabler.java PRE-CREATION
tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java a844aa0f8
Diff: https://reviews.apache.org/r/62350/diff/2/
Changes: https://reviews.apache.org/r/62350/diff/1-2/
Testing
-------
Thanks,
Peter Bacsko
Re: Review Request 62350: OOZIE-3054 Disable erasure coding for
sharelib if Oozie runs on Hadoop 3
Posted by Robert Kanter <rk...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62350/#review185501
-----------------------------------------------------------
tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java
Lines 35 (patched)
<https://reviews.apache.org/r/62350/#comment261799>
Let's call this something more specific to what it's doing. Maybe "ECPolicyDisabler" or something like that.
tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java
Lines 44 (patched)
<https://reviews.apache.org/r/62350/#comment261801>
"Found Hadoop that supports Erasure Coding. Trying to disable Erasure Coding for " + path
tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java
Lines 58 (patched)
<https://reviews.apache.org/r/62350/#comment261802>
System.out.println("Done");
tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java
Lines 61 (patched)
<https://reviews.apache.org/r/62350/#comment261803>
"Found Hadoop that does not support Erasure Coding. Not taking any action."
tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java
Lines 66 (patched)
<https://reviews.apache.org/r/62350/#comment261800>
Let's rename this to ``supportsErasureCoding()``
tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java
Lines 68 (patched)
<https://reviews.apache.org/r/62350/#comment261805>
This is for the sharelib upload tool, so it will only happen once per JVM. So I don't think we need to bother caching it.
- Robert Kanter
On Sept. 15, 2017, 12:07 p.m., Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62350/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2017, 12:07 p.m.)
>
>
> Review request for oozie, András Piros, Peter Cseh, and Robert Kanter.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> See https://issues.apache.org/jira/browse/OOZIE-3054
>
>
> Diffs
> -----
>
> tools/src/main/java/org/apache/oozie/tools/Hadoop3Support.java PRE-CREATION
> tools/src/main/java/org/apache/oozie/tools/OozieSharelibCLI.java a844aa0f8
>
>
> Diff: https://reviews.apache.org/r/62350/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Peter Bacsko
>
>