Re: Review Request 52737: OOZIE-2698 OYA: Refactor LauncherAM to make it more testable

2016-10-15 Thread Peter Bacsko 

---
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

2016-10-15 Thread Peter Bacsko 

---
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

2016-10-14 Thread András Piros 

---
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

2016-10-14 Thread Peter Bacsko 

---
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

2016-10-14 Thread Peter Bacsko 


- 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

2016-10-14 Thread Peter Bacsko 


> 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > `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 
>

Re: Review Request 52737: OOZIE-2698 OYA: Refactor LauncherAM to make it more testable

2016-10-13 Thread Attila Sasvari 


> 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
> > 
> >
> > The second parameter of checkNotNull is incorrect, it is a String
> > that is used in the detail message of the exception:
> > 
> > ```
> > public static  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

2016-10-13 Thread Attila Sasvari 

---
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)


The second parameter of checkNotNull is incorrect, it is a String
that is used in the detail message of the exception:

```
public static  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

2016-10-12 Thread András Piros 

---
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)


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)


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)


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)


Consider using the existing `ErrorCode` and `XException` classes instead.



sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java
 (line 38)


I'd separate concerns to different classes.



sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java
 (line 43)


Better would be `"seqFileWriterFactory should not be null"`.



sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java
 (line 44)


Better would be `"ugi should not be null"`.



sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java
 (line 57)


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)


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)


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)


Dead code, pls. remove.



sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 
(lines 350 - 352)


Dead code, pls. remove.



sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 
(lines 355 - 356)


Dead code, pls. remove.



sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 
(lines 373 - 375)


Dead code, pls. remove.



sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 
(line 425)


Why not use `AtomicReference#compareAndSet()` instead?



sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 
(lines 475 - 476)


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)


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)


Why is `operation` a `String`?



sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java
 (lines 69 - 74)


`switch case default` would be nicer here.



sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/PrepareActionsHandler.java
 (line 89)

Re: Review Request 52737: OOZIE-2698 OYA: Refactor LauncherAM to make it more testable

2016-10-11 Thread Peter Bacsko 

---
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)


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)


TODO: add JavaDoc



sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/HdfsAndLocalFSOperations.java
 (line 102)


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)


TODO: add JavaDoc



sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 
(line 235)


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)


This map will be inspected in unit tests



sharelib/oozie/src/main/java/org/apache/oozie/action/hadoop/LauncherAM.java 
(line 292)


The reason for the factory is that the value "6" 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)


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

2016-10-11 Thread Peter Bacsko 

---
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

Review Request 52737: OOZIE-2698 OYA: Refactor LauncherAM to make it more testable

2016-10-11 Thread Peter Bacsko 

---
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