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 2016/10/11 13:41:37 UTC
Review Request 52737: OOZIE-2698 OYA: Refactor LauncherAM to make it
more testable
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52737/
-----------------------------------------------------------
Review request for oozie, Andr�s Piros, Attila Sasvari, Peter Cseh, and Robert Kanter.
Repository: oozie-git
Description
-------
This review contains a suggested approach for refactoring LauncherAM on the OYA branch.
The main idea is to inject all dependencies of the class via the constructor. In some cases, this requires factory objects. Note that this is necessary because in unit tests, we don't want to create actual resources such as UGI or async ResourceManager callback.
In the tests, we can instantiate this class with mocks. The testability is greatly enhanced because various checks and verifications can be performed on the mocks. Tests are currently not in this review (see https://issues.apache.org/jira/browse/OOZIE-2685).
Diffs
-----
core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java ed29299
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMClientAsyncFactory.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ErrorHolder.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 85d78c6
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifier.java 23648b8
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifierFactory.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java 4a51d48
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/SequenceFileWriterFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/52737/diff/
Testing
-------
Unit tests are being written right now.
Thanks,
Peter Bacsko
Re: Review Request 52737: OOZIE-2698 OYA: Refactor LauncherAM to make
it more testable
Posted by Attila Sasvari <as...@cloudera.com>.
> On Oct. 13, 2016, 6:12 p.m., Attila Sasvari wrote:
> > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java, line 103
> > <https://reviews.apache.org/r/52737/diff/1/?file=1530512#file1530512line103>
> >
> > The second parameter of checkNotNull is incorrect, it is a String
> > that is used in the detail message of the exception:
> >
> > ```
> > public static <T> T checkNotNull(T o, String name) throws NullPointerException {
> > if(o == null) {
> > throw new NullPointerException(name + " should not be null");
> > } else {
> > return o;
> > }
> > }
> > ```
false alarm. this checknotnull was from parquet...
- Attila
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52737/#review152545
-----------------------------------------------------------
On Oct. 11, 2016, 1:42 p.m., Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52737/
> -----------------------------------------------------------
>
> (Updated Oct. 11, 2016, 1:42 p.m.)
>
>
> Review request for oozie, Andr�s Piros, Attila Sasvari, Peter Cseh, and Robert Kanter.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> This review contains a suggested approach for refactoring LauncherAM on the OYA branch.
>
> The main idea is to inject all dependencies of the class via the constructor. In some cases, this requires factory objects. Note that this is necessary because in unit tests, because we don't want to create actual resources such as UGI or async ResourceManager callback.
>
> In the tests, we can instantiate this class with mocks. The testability is greatly enhanced because various checks and verifications can be performed on the mocks. Tests are currently not in this review (see https://issues.apache.org/jira/browse/OOZIE-2685).
>
>
> Diffs
> -----
>
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java ed29299
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMClientAsyncFactory.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ErrorHolder.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 85d78c6
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifier.java 23648b8
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifierFactory.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java 4a51d48
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/SequenceFileWriterFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52737/diff/
>
>
> Testing
> -------
>
> Unit tests are being written right now.
>
>
> Thanks,
>
> Peter Bacsko
>
>
Re: Review Request 52737: OOZIE-2698 OYA: Refactor LauncherAM to make
it more testable
Posted by Attila Sasvari <as...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52737/#review152545
-----------------------------------------------------------
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java (line 94)
<https://reviews.apache.org/r/52737/#comment221587>
The second parameter of checkNotNull is incorrect, it is a String
that is used in the detail message of the exception:
```
public static <T> T checkNotNull(T o, String name) throws NullPointerException {
if(o == null) {
throw new NullPointerException(name + " should not be null");
} else {
return o;
}
}
```
- Attila Sasvari
On Oct. 11, 2016, 1:42 p.m., Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52737/
> -----------------------------------------------------------
>
> (Updated Oct. 11, 2016, 1:42 p.m.)
>
>
> Review request for oozie, Andr�s Piros, Attila Sasvari, Peter Cseh, and Robert Kanter.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> This review contains a suggested approach for refactoring LauncherAM on the OYA branch.
>
> The main idea is to inject all dependencies of the class via the constructor. In some cases, this requires factory objects. Note that this is necessary because in unit tests, because we don't want to create actual resources such as UGI or async ResourceManager callback.
>
> In the tests, we can instantiate this class with mocks. The testability is greatly enhanced because various checks and verifications can be performed on the mocks. Tests are currently not in this review (see https://issues.apache.org/jira/browse/OOZIE-2685).
>
>
> Diffs
> -----
>
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java ed29299
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMClientAsyncFactory.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ErrorHolder.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 85d78c6
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifier.java 23648b8
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifierFactory.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java 4a51d48
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/SequenceFileWriterFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52737/diff/
>
>
> Testing
> -------
>
> Unit tests are being written right now.
>
>
> Thanks,
>
> Peter Bacsko
>
>
Re: Review Request 52737: OOZIE-2698 OYA: Refactor LauncherAM to make
it more testable
Posted by András Piros <an...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52737/#review152652
-----------------------------------------------------------
Ship it!
Ship It!
- Andr�s Piros
On Oct. 14, 2016, 10:01 a.m., Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52737/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2016, 10:01 a.m.)
>
>
> Review request for oozie, Andr�s Piros, Attila Sasvari, Peter Cseh, and Robert Kanter.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> This review contains a suggested approach for refactoring LauncherAM on the OYA branch.
>
> The main idea is to inject all dependencies of the class via the constructor. In some cases, this requires factory objects. Note that this is necessary because in unit tests, because we don't want to create actual resources such as UGI or async ResourceManager callback.
>
> In the tests, we can instantiate this class with mocks. The testability is greatly enhanced because various checks and verifications can be performed on the mocks. Tests are currently not in this review (see https://issues.apache.org/jira/browse/OOZIE-2685).
>
>
> Diffs
> -----
>
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java ed29299
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMClientAsyncFactory.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ErrorHolder.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsOperations.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 85d78c6
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifier.java 23648b8
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifierFactory.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LocalFsOperations.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java 4a51d48
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/SequenceFileWriterFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52737/diff/
>
>
> Testing
> -------
>
> Unit tests are being written right now.
>
>
> Thanks,
>
> Peter Bacsko
>
>
Re: Review Request 52737: OOZIE-2698 OYA: Refactor LauncherAM to make
it more testable
Posted by Peter Bacsko <pb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52737/
-----------------------------------------------------------
(Updated okt. 15, 2016, 11:41 de)
Review request for oozie, Andr�s Piros, Attila Sasvari, Peter Cseh, and Robert Kanter.
Changes
-------
Some minor changes.
Repository: oozie-git
Description
-------
This review contains a suggested approach for refactoring LauncherAM on the OYA branch.
The main idea is to inject all dependencies of the class via the constructor. In some cases, this requires factory objects. Note that this is necessary because in unit tests, because we don't want to create actual resources such as UGI or async ResourceManager callback.
In the tests, we can instantiate this class with mocks. The testability is greatly enhanced because various checks and verifications can be performed on the mocks. Tests are currently not in this review (see https://issues.apache.org/jira/browse/OOZIE-2685).
Diffs (updated)
-----
core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java ed29299
core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAMCallbackNotifier.java 9ba04da
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMClientAsyncFactory.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ErrorHolder.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsOperations.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 85d78c6
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifier.java 23648b8
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifierFactory.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LocalFsOperations.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java 4a51d48
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/SequenceFileWriterFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/52737/diff/
Testing
-------
Unit tests are being written right now.
Thanks,
Peter Bacsko
Re: Review Request 52737: OOZIE-2698 OYA: Refactor LauncherAM to make
it more testable
Posted by Peter Bacsko <pb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52737/
-----------------------------------------------------------
(Updated okt. 15, 2016, 11:33 de)
Review request for oozie, Andr�s Piros, Attila Sasvari, Peter Cseh, and Robert Kanter.
Changes
-------
Further changes:
- no longer processing args[] is LauncherAM because they're unused
- replaced FinalApplicationStatus usage, and a new enum has been introduced.
Reason for the new enum: FinalAppStatus doesn't have RUNNING value. It's necessary when we submit MR jobs. So I created a new OozieActionResult which has RUNNING/SUCCEEDED/FAILED state.
The responsibility of sending back "RUNNING" state was moved from LauncherAMCallbackNotifier to LauncherAM.
Repository: oozie-git
Description
-------
This review contains a suggested approach for refactoring LauncherAM on the OYA branch.
The main idea is to inject all dependencies of the class via the constructor. In some cases, this requires factory objects. Note that this is necessary because in unit tests, because we don't want to create actual resources such as UGI or async ResourceManager callback.
In the tests, we can instantiate this class with mocks. The testability is greatly enhanced because various checks and verifications can be performed on the mocks. Tests are currently not in this review (see https://issues.apache.org/jira/browse/OOZIE-2685).
Diffs (updated)
-----
core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java ed29299
core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAMCallbackNotifier.java 9ba04da
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMClientAsyncFactory.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ErrorHolder.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsOperations.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 85d78c6
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifier.java 23648b8
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifierFactory.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LocalFsOperations.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java 4a51d48
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/SequenceFileWriterFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/52737/diff/
Testing
-------
Unit tests are being written right now.
Thanks,
Peter Bacsko
Re: Review Request 52737: OOZIE-2698 OYA: Refactor LauncherAM to make
it more testable
Posted by Peter Bacsko <pb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52737/
-----------------------------------------------------------
(Updated okt. 14, 2016, 10:01 de)
Review request for oozie, Andr�s Piros, Attila Sasvari, Peter Cseh, and Robert Kanter.
Repository: oozie-git
Description
-------
This review contains a suggested approach for refactoring LauncherAM on the OYA branch.
The main idea is to inject all dependencies of the class via the constructor. In some cases, this requires factory objects. Note that this is necessary because in unit tests, because we don't want to create actual resources such as UGI or async ResourceManager callback.
In the tests, we can instantiate this class with mocks. The testability is greatly enhanced because various checks and verifications can be performed on the mocks. Tests are currently not in this review (see https://issues.apache.org/jira/browse/OOZIE-2685).
Diffs (updated)
-----
core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java ed29299
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMClientAsyncFactory.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ErrorHolder.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsOperations.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 85d78c6
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifier.java 23648b8
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifierFactory.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LocalFsOperations.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java 4a51d48
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/SequenceFileWriterFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/52737/diff/
Testing
-------
Unit tests are being written right now.
Thanks,
Peter Bacsko
Re: Review Request 52737: OOZIE-2698 OYA: Refactor LauncherAM to make
it more testable
Posted by Peter Bacsko <pb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52737/#review152128
-----------------------------------------------------------
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java (line 27)
<https://reviews.apache.org/r/52737/#comment220904>
This class called failLauncer() before. It's unsafe because these calls are invoked from a separate thread. IMO It's better to check errors synchronously at the end of LauncherAM.run().
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java (line 81)
<https://reviews.apache.org/r/52737/#comment220902>
TODO: add JavaDoc
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java (line 102)
<https://reviews.apache.org/r/52737/#comment220897>
We can move this to the SeqFileWriter factory.
A better name would be createReader not getReader.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java (line 106)
<https://reviews.apache.org/r/52737/#comment220903>
TODO: add JavaDoc
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java (line 235)
<https://reviews.apache.org/r/52737/#comment220901>
The callback notifier creates URL connections, which we don't want here. Since it also requires the launcher job conf, it cannot be instantiated in main() -- so we create it with a factory.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java (line 243)
<https://reviews.apache.org/r/52737/#comment220898>
This map will be inspected in unit tests
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java (line 292)
<https://reviews.apache.org/r/52737/#comment220899>
The reason for the factory is that the value "60000" will come from the launcher job xml. It's easier from failure handling POV if the xml is loaded in run(), not in main().
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java (line 482)
<https://reviews.apache.org/r/52737/#comment220900>
TODO: actionData will be removed
- Peter Bacsko
On okt. 11, 2016, 1:42 du, Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52737/
> -----------------------------------------------------------
>
> (Updated okt. 11, 2016, 1:42 du)
>
>
> Review request for oozie, Andr�s Piros, Attila Sasvari, Peter Cseh, and Robert Kanter.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> This review contains a suggested approach for refactoring LauncherAM on the OYA branch.
>
> The main idea is to inject all dependencies of the class via the constructor. In some cases, this requires factory objects. Note that this is necessary because in unit tests, because we don't want to create actual resources such as UGI or async ResourceManager callback.
>
> In the tests, we can instantiate this class with mocks. The testability is greatly enhanced because various checks and verifications can be performed on the mocks. Tests are currently not in this review (see https://issues.apache.org/jira/browse/OOZIE-2685).
>
>
> Diffs
> -----
>
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java ed29299
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMClientAsyncFactory.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ErrorHolder.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 85d78c6
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifier.java 23648b8
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifierFactory.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java 4a51d48
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/SequenceFileWriterFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52737/diff/
>
>
> Testing
> -------
>
> Unit tests are being written right now.
>
>
> Thanks,
>
> Peter Bacsko
>
>
Re: Review Request 52737: OOZIE-2698 OYA: Refactor LauncherAM to make
it more testable
Posted by Peter Bacsko <pb...@cloudera.com>.
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52737/#review152319
-----------------------------------------------------------
On okt. 11, 2016, 1:42 du, Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52737/
> -----------------------------------------------------------
>
> (Updated okt. 11, 2016, 1:42 du)
>
>
> Review request for oozie, Andr�s Piros, Attila Sasvari, Peter Cseh, and Robert Kanter.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> This review contains a suggested approach for refactoring LauncherAM on the OYA branch.
>
> The main idea is to inject all dependencies of the class via the constructor. In some cases, this requires factory objects. Note that this is necessary because in unit tests, because we don't want to create actual resources such as UGI or async ResourceManager callback.
>
> In the tests, we can instantiate this class with mocks. The testability is greatly enhanced because various checks and verifications can be performed on the mocks. Tests are currently not in this review (see https://issues.apache.org/jira/browse/OOZIE-2685).
>
>
> Diffs
> -----
>
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java ed29299
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMClientAsyncFactory.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ErrorHolder.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 85d78c6
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifier.java 23648b8
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifierFactory.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java 4a51d48
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/SequenceFileWriterFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52737/diff/
>
>
> Testing
> -------
>
> Unit tests are being written right now.
>
>
> Thanks,
>
> Peter Bacsko
>
>
Re: Review Request 52737: OOZIE-2698 OYA: Refactor LauncherAM to make
it more testable
Posted by Peter Bacsko <pb...@cloudera.com>.
> On okt. 12, 2016, 4:03 du, Andr�s Piros wrote:
> > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java, line 41
> > <https://reviews.apache.org/r/52737/diff/1/?file=1530508#file1530508line41>
> >
> > Why `synchronized`? I don't see we reach out to global state, or shared resources.
This method is called from a separate thread asynchronously, then we check the error from main. Perhaps I should add some comments.
> On okt. 12, 2016, 4:03 du, Andr�s Piros wrote:
> > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java, line 59
> > <https://reviews.apache.org/r/52737/diff/1/?file=1530508#file1530508line59>
> >
> > Why `synchronized`? I don't see we reach out to global state, or shared resources.
See my comment above.
> On okt. 12, 2016, 4:03 du, Andr�s Piros wrote:
> > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java, line 68
> > <https://reviews.apache.org/r/52737/diff/1/?file=1530508#file1530508line68>
> >
> > Why `synchronized`? I don't see we reach out to global state, or shared resources.
See my comment above.
> On okt. 12, 2016, 4:03 du, Andr�s Piros wrote:
> > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java, line 57
> > <https://reviews.apache.org/r/52737/diff/1/?file=1530511#file1530511line57>
> >
> > What about using `Files#walkFileTree` instead?
> >
> > https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#walkFileTree(java.nio.file.Path,%20java.util.Set,%20int,%20java.nio.file.FileVisitor)
This sounds like a good idea:)
> On okt. 12, 2016, 4:03 du, Andr�s Piros wrote:
> > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java, line 470
> > <https://reviews.apache.org/r/52737/diff/1/?file=1530512#file1530512line470>
> >
> > Why not use `AtomicReference#compareAndSet()` instead?
We have to use some holder object here because we mutate state inside an anon class. A simple boolean wouldn't do it. MutableObject<Boolean> probably would be better here but it's in commons-lang3 and don't think it's available on the classpath.
I'm going to add a comment to clarify this.
> On okt. 12, 2016, 4:03 du, Andr�s Piros wrote:
> > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java, line 67
> > <https://reviews.apache.org/r/52737/diff/1/?file=1530516#file1530516line67>
> >
> > Why is `operation` a `String`?
Fair point: this is old code - didn't want to modify it (yet!).
> On okt. 12, 2016, 4:03 du, Andr�s Piros wrote:
> > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java, line 38
> > <https://reviews.apache.org/r/52737/diff/1/?file=1530511#file1530511line38>
> >
> > I'd separate concerns to different classes.
Makes sense - done
> On okt. 12, 2016, 4:03 du, Andr�s Piros wrote:
> > sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java, line 89
> > <https://reviews.apache.org/r/52737/diff/1/?file=1530516#file1530516line89>
> >
> > `Charsets.UTF_8.toString()`
Done
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52737/#review152319
-----------------------------------------------------------
On okt. 11, 2016, 1:42 du, Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52737/
> -----------------------------------------------------------
>
> (Updated okt. 11, 2016, 1:42 du)
>
>
> Review request for oozie, Andr�s Piros, Attila Sasvari, Peter Cseh, and Robert Kanter.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> This review contains a suggested approach for refactoring LauncherAM on the OYA branch.
>
> The main idea is to inject all dependencies of the class via the constructor. In some cases, this requires factory objects. Note that this is necessary because in unit tests, because we don't want to create actual resources such as UGI or async ResourceManager callback.
>
> In the tests, we can instantiate this class with mocks. The testability is greatly enhanced because various checks and verifications can be performed on the mocks. Tests are currently not in this review (see https://issues.apache.org/jira/browse/OOZIE-2685).
>
>
> Diffs
> -----
>
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java ed29299
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMClientAsyncFactory.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ErrorHolder.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 85d78c6
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifier.java 23648b8
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifierFactory.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java 4a51d48
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/SequenceFileWriterFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52737/diff/
>
>
> Testing
> -------
>
> Unit tests are being written right now.
>
>
> Thanks,
>
> Peter Bacsko
>
>
Re: Review Request 52737: OOZIE-2698 OYA: Refactor LauncherAM to make
it more testable
Posted by András Piros <an...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52737/#review152319
-----------------------------------------------------------
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java (line 41)
<https://reviews.apache.org/r/52737/#comment221291>
Why `synchronized`? I don't see we reach out to global state, or shared resources.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java (line 59)
<https://reviews.apache.org/r/52737/#comment221292>
Why `synchronized`? I don't see we reach out to global state, or shared resources.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java (line 68)
<https://reviews.apache.org/r/52737/#comment221296>
Why `synchronized`? I don't see we reach out to global state, or shared resources.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ErrorHolder.java (line 20)
<https://reviews.apache.org/r/52737/#comment221297>
Consider using the existing `ErrorCode` and `XException` classes instead.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java (line 38)
<https://reviews.apache.org/r/52737/#comment221309>
I'd separate concerns to different classes.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java (line 43)
<https://reviews.apache.org/r/52737/#comment221301>
Better would be `"seqFileWriterFactory should not be null"`.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java (line 44)
<https://reviews.apache.org/r/52737/#comment221302>
Better would be `"ugi should not be null"`.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java (line 57)
<https://reviews.apache.org/r/52737/#comment221305>
What about using `Files#walkFileTree` instead?
https://docs.oracle.com/javase/7/docs/api/java/nio/file/Files.html#walkFileTree(java.nio.file.Path,%20java.util.Set,%20int,%20java.nio.file.FileVisitor)
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java (line 81)
<https://reviews.apache.org/r/52737/#comment221306>
What about using Guava's `Files#toString()`?
https://google.github.io/guava/releases/19.0/api/docs/com/google/common/io/Files.html#toString(java.io.File, java.nio.charset.Charset)
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java (line 102)
<https://reviews.apache.org/r/52737/#comment221307>
What about using Guava's `Files#toString()`?
https://google.github.io/guava/releases/19.0/api/docs/com/google/common/io/Files.html#toString(java.io.File, java.nio.charset.Charset)
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java (lines 350 - 352)
<https://reviews.apache.org/r/52737/#comment221311>
Dead code, pls. remove.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java (lines 350 - 352)
<https://reviews.apache.org/r/52737/#comment221314>
Dead code, pls. remove.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java (lines 355 - 356)
<https://reviews.apache.org/r/52737/#comment221312>
Dead code, pls. remove.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java (lines 373 - 375)
<https://reviews.apache.org/r/52737/#comment221313>
Dead code, pls. remove.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java (line 425)
<https://reviews.apache.org/r/52737/#comment221315>
Why not use `AtomicReference#compareAndSet()` instead?
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java (lines 475 - 476)
<https://reviews.apache.org/r/52737/#comment221318>
Consider Guava's `Files#toString()`:
https://google.github.io/guava/releases/19.0/api/docs/com/google/common/io/Files.html#toString(java.io.File, java.nio.charset.Charset)
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java (lines 512 - 514)
<https://reviews.apache.org/r/52737/#comment221320>
Consider Guava's `Files#toString()`:
https://google.github.io/guava/releases/19.0/api/docs/com/google/common/io/Files.html#toString(java.io.File, java.nio.charset.Charset)
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java (line 67)
<https://reviews.apache.org/r/52737/#comment221322>
Why is `operation` a `String`?
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java (lines 69 - 74)
<https://reviews.apache.org/r/52737/#comment221323>
`switch case default` would be nicer here.
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java (line 89)
<https://reviews.apache.org/r/52737/#comment221321>
`Charsets.UTF_8.toString()`
- Andr�s Piros
On Oct. 11, 2016, 1:42 p.m., Peter Bacsko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52737/
> -----------------------------------------------------------
>
> (Updated Oct. 11, 2016, 1:42 p.m.)
>
>
> Review request for oozie, Andr�s Piros, Attila Sasvari, Peter Cseh, and Robert Kanter.
>
>
> Repository: oozie-git
>
>
> Description
> -------
>
> This review contains a suggested approach for refactoring LauncherAM on the OYA branch.
>
> The main idea is to inject all dependencies of the class via the constructor. In some cases, this requires factory objects. Note that this is necessary because in unit tests, because we don't want to create actual resources such as UGI or async ResourceManager callback.
>
> In the tests, we can instantiate this class with mocks. The testability is greatly enhanced because various checks and verifications can be performed on the mocks. Tests are currently not in this review (see https://issues.apache.org/jira/browse/OOZIE-2685).
>
>
> Diffs
> -----
>
> core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java ed29299
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMClientAsyncFactory.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ErrorHolder.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 85d78c6
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifier.java 23648b8
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifierFactory.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java 4a51d48
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java PRE-CREATION
> sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/SequenceFileWriterFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/52737/diff/
>
>
> Testing
> -------
>
> Unit tests are being written right now.
>
>
> Thanks,
>
> Peter Bacsko
>
>
Re: Review Request 52737: OOZIE-2698 OYA: Refactor LauncherAM to make
it more testable
Posted by Peter Bacsko <pb...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52737/
-----------------------------------------------------------
(Updated okt. 11, 2016, 1:42 du)
Review request for oozie, Andr�s Piros, Attila Sasvari, Peter Cseh, and Robert Kanter.
Repository: oozie-git
Description (updated)
-------
This review contains a suggested approach for refactoring LauncherAM on the OYA branch.
The main idea is to inject all dependencies of the class via the constructor. In some cases, this requires factory objects. Note that this is necessary because in unit tests, because we don't want to create actual resources such as UGI or async ResourceManager callback.
In the tests, we can instantiate this class with mocks. The testability is greatly enhanced because various checks and verifications can be performed on the mocks. Tests are currently not in this review (see https://issues.apache.org/jira/browse/OOZIE-2685).
Diffs
-----
core/src/test/java/org/apache/oozie/action/hadoop/TestLauncherAM.java ed29299
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMCallBackHandler.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/AMRMClientAsyncFactory.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/ErrorHolder.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 85d78c6
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifier.java 23648b8
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAMCallbackNotifierFactory.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsDriver.java 4a51d48
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java PRE-CREATION
sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/SequenceFileWriterFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/52737/diff/
Testing
-------
Unit tests are being written right now.
Thanks,
Peter Bacsko