Re: Review Request 57870: OOZIE-2827 More directly view of the coordinator’s history from perspective of workflow action.

2017-04-13 Thread Peter Bacsko 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57870/#review171880
---


Ship it!




Ship It!

- Peter Bacsko


On ápr. 6, 2017, 2:34 du, Alonzo Zhou wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57870/
> ---
> 
> (Updated ápr. 6, 2017, 2:34 du)
> 
> 
> Review request for oozie.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> More detailed view of the coordinator’s history can be observed from 
> perspective of  workflow action.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/client/CoordinatorWfAction.java 
> PRE-CREATION 
>   client/src/main/java/org/apache/oozie/client/rest/JsonTags.java ca168e0 
>   client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 
> 8ddb1f8 
>   core/src/main/java/org/apache/oozie/CoordinatorEngine.java 2f9f822 
>   core/src/main/java/org/apache/oozie/CoordinatorWfActionBean.java 
> PRE-CREATION 
>   core/src/main/java/org/apache/oozie/ErrorCode.java b03ad06 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
>  PRE-CREATION 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/WorkflowActionGetJPAExecutor.java
>  0b7f50d 
>   core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java 03acbc1 
>   core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java 6c30f5d 
>   core/src/test/java/org/apache/oozie/client/TestOozieCLI.java 564db2a 
>   
> core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java 
> 4fc8653 
>   core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java fb203a6 
> 
> 
> Diff: https://reviews.apache.org/r/57870/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alonzo Zhou
> 
>

Re: Review Request 57870: OOZIE-2827 More directly view of the coordinator’s history from perspective of workflow action.

2017-04-13 Thread András Piros 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57870/#review171870
---


Ship it!




Ship It!

- András Piros


On April 6, 2017, 2:34 p.m., Alonzo Zhou wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57870/
> ---
> 
> (Updated April 6, 2017, 2:34 p.m.)
> 
> 
> Review request for oozie.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> More detailed view of the coordinator’s history can be observed from 
> perspective of  workflow action.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/client/CoordinatorWfAction.java 
> PRE-CREATION 
>   client/src/main/java/org/apache/oozie/client/rest/JsonTags.java ca168e0 
>   client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 
> 8ddb1f8 
>   core/src/main/java/org/apache/oozie/CoordinatorEngine.java 2f9f822 
>   core/src/main/java/org/apache/oozie/CoordinatorWfActionBean.java 
> PRE-CREATION 
>   core/src/main/java/org/apache/oozie/ErrorCode.java b03ad06 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
>  PRE-CREATION 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/WorkflowActionGetJPAExecutor.java
>  0b7f50d 
>   core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java 03acbc1 
>   core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java 6c30f5d 
>   core/src/test/java/org/apache/oozie/client/TestOozieCLI.java 564db2a 
>   
> core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java 
> 4fc8653 
>   core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java fb203a6 
> 
> 
> Diff: https://reviews.apache.org/r/57870/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alonzo Zhou
> 
>

Re: Review Request 57870: OOZIE-2827 More directly view of the coordinator’s history from perspective of workflow action.

2017-04-06 Thread Alonzo Zhou 


> On 四月 3, 2017, 10:23 a.m., Peter Bacsko wrote:
> > core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
> > Lines 81 (patched)
> > 
> >
> > Are we sure that the default, uninitialized instance is good enough?
> > 
> > So let's say we store the empty bean as a placeholder. How will that be 
> > returned to the user?
> > 
> > As I can see, status will be PREP, wfId is null, id is null, everything 
> > else is pretty much null.
> > 
> > Is this sufficient for us? Won't that be misleading?

hi, i added a class CoordinatorWfActionBean as an element in the returned list, 
which includes a WorkflowActionBean, an int that corresponds to the 
coordinatorAction serial number, and a string to explain why the 
WorkflowActionBean is null.
The packaging looks a bit more robustness and friendly. could you review it for 
me ? thanks.


- Alonzo


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57870/#review170838
---


On 四月 6, 2017, 2:34 p.m., Alonzo Zhou wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57870/
> ---
> 
> (Updated 四月 6, 2017, 2:34 p.m.)
> 
> 
> Review request for oozie.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> More detailed view of the coordinator’s history can be observed from 
> perspective of  workflow action.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/client/CoordinatorWfAction.java 
> PRE-CREATION 
>   client/src/main/java/org/apache/oozie/client/rest/JsonTags.java ca168e0 
>   client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 
> 8ddb1f8 
>   core/src/main/java/org/apache/oozie/CoordinatorEngine.java 2f9f822 
>   core/src/main/java/org/apache/oozie/CoordinatorWfActionBean.java 
> PRE-CREATION 
>   core/src/main/java/org/apache/oozie/ErrorCode.java b03ad06 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
>  PRE-CREATION 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/WorkflowActionGetJPAExecutor.java
>  0b7f50d 
>   core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java 03acbc1 
>   core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java 6c30f5d 
>   core/src/test/java/org/apache/oozie/client/TestOozieCLI.java 564db2a 
>   
> core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java 
> 4fc8653 
>   core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java fb203a6 
> 
> 
> Diff: https://reviews.apache.org/r/57870/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alonzo Zhou
> 
>

Re: Review Request 57870: OOZIE-2827 More directly view of the coordinator’s history from perspective of workflow action.

2017-04-06 Thread Alonzo Zhou 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57870/
---

(Updated 四月 6, 2017, 2:34 p.m.)


Review request for oozie.


Changes
---

1 refactor the test so that they run as a single testcase 
(TestCoordWfActionInfoXCommand, TestV2JobServlet)
2 Added a class CoordinatorWfActionBean as an element in the returned list


Repository: oozie-git


Description
---

More detailed view of the coordinator’s history can be observed from 
perspective of  workflow action.


Diffs (updated)
-

  client/src/main/java/org/apache/oozie/client/CoordinatorWfAction.java 
PRE-CREATION 
  client/src/main/java/org/apache/oozie/client/rest/JsonTags.java ca168e0 
  client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 8ddb1f8 
  core/src/main/java/org/apache/oozie/CoordinatorEngine.java 2f9f822 
  core/src/main/java/org/apache/oozie/CoordinatorWfActionBean.java PRE-CREATION 
  core/src/main/java/org/apache/oozie/ErrorCode.java b03ad06 
  
core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
 PRE-CREATION 
  
core/src/main/java/org/apache/oozie/executor/jpa/WorkflowActionGetJPAExecutor.java
 0b7f50d 
  core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java 03acbc1 
  core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java 6c30f5d 
  core/src/test/java/org/apache/oozie/client/TestOozieCLI.java 564db2a 
  
core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java
 PRE-CREATION 
  core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java 
4fc8653 
  core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java fb203a6 


Diff: https://reviews.apache.org/r/57870/diff/5/

Changes: https://reviews.apache.org/r/57870/diff/4-5/


Testing
---


Thanks,

Alonzo Zhou

Re: Review Request 57870: OOZIE-2827 More directly view of the coordinator’s history from perspective of workflow action.

2017-04-03 Thread Peter Bacsko 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57870/#review170838
---




core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
Lines 81 (patched)


Are we sure that the default, uninitialized instance is good enough?

So let's say we store the empty bean as a placeholder. How will that be 
returned to the user?

As I can see, status will be PREP, wfId is null, id is null, everything 
else is pretty much null.

Is this sufficient for us? Won't that be misleading?



core/src/main/java/org/apache/oozie/executor/jpa/WorkflowActionGetJPAExecutor.java
Lines 82 (patched)


In this case please return null instead of creating a new bean and handle 
the null case on the caller side.



core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java
Lines 63 (patched)


Is it possible to refactor the test so that they run as a single testcase?



core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java
Lines 262 (patched)


How much overhead does this test have if we run all subtests as a separate 
testcase?

Having them separately would be much better.


- Peter Bacsko


On ápr. 1, 2017, 11:31 de, Alonzo Zhou wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57870/
> ---
> 
> (Updated ápr. 1, 2017, 11:31 de)
> 
> 
> Review request for oozie.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> More detailed view of the coordinator’s history can be observed from 
> perspective of  workflow action.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 
> 8ddb1f8 
>   core/src/main/java/org/apache/oozie/CoordinatorEngine.java 2f9f822 
>   core/src/main/java/org/apache/oozie/ErrorCode.java b03ad06 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
>  PRE-CREATION 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/WorkflowActionGetJPAExecutor.java
>  0b7f50d 
>   core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java 03acbc1 
>   core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java 6c30f5d 
>   core/src/test/java/org/apache/oozie/client/TestOozieCLI.java 564db2a 
>   
> core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java 
> 4fc8653 
>   core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java fb203a6 
> 
> 
> Diff: https://reviews.apache.org/r/57870/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alonzo Zhou
> 
>

Re: Review Request 57870: OOZIE-2827 More directly view of the coordinator’s history from perspective of workflow action.

2017-04-01 Thread Alonzo Zhou 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57870/
---

(Updated 四月 1, 2017, 11:31 a.m.)


Review request for oozie.


Changes
---

1 add comment to the ambiguous method with empty body
2 Use empty WorkflowActionBean instead of null as placeholder element
3 Make WorkflowActionGetJPAExecutor be able to handle both "acceptable null" 
and "null exception"


Repository: oozie-git


Description
---

More detailed view of the coordinator’s history can be observed from 
perspective of  workflow action.


Diffs (updated)
-

  client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 8ddb1f8 
  core/src/main/java/org/apache/oozie/CoordinatorEngine.java 2f9f822 
  core/src/main/java/org/apache/oozie/ErrorCode.java b03ad06 
  
core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
 PRE-CREATION 
  
core/src/main/java/org/apache/oozie/executor/jpa/WorkflowActionGetJPAExecutor.java
 0b7f50d 
  core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java 03acbc1 
  core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java 6c30f5d 
  core/src/test/java/org/apache/oozie/client/TestOozieCLI.java 564db2a 
  
core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java
 PRE-CREATION 
  core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java 
4fc8653 
  core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java fb203a6 


Diff: https://reviews.apache.org/r/57870/diff/4/

Changes: https://reviews.apache.org/r/57870/diff/3-4/


Testing
---


Thanks,

Alonzo Zhou

Re: Review Request 57870: OOZIE-2827 More directly view of the coordinator’s history from perspective of workflow action.

2017-03-31 Thread András Piros 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57870/#review170695
---


Ship it!




Ship It!

- András Piros


On March 28, 2017, 5:41 p.m., Alonzo Zhou wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57870/
> ---
> 
> (Updated March 28, 2017, 5:41 p.m.)
> 
> 
> Review request for oozie.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> More detailed view of the coordinator’s history can be observed from 
> perspective of  workflow action.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 
> 8ddb1f8 
>   core/src/main/java/org/apache/oozie/CoordinatorEngine.java 2f9f822 
>   core/src/main/java/org/apache/oozie/ErrorCode.java b03ad06 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
>  PRE-CREATION 
>   core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java 03acbc1 
>   core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java 9356768 
>   core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java 6c30f5d 
>   core/src/test/java/org/apache/oozie/client/TestOozieCLI.java 564db2a 
>   
> core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java 
> 4fc8653 
>   core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java fb203a6 
> 
> 
> Diff: https://reviews.apache.org/r/57870/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alonzo Zhou
> 
>

Re: Review Request 57870: OOZIE-2827 More directly view of the coordinator’s history from perspective of workflow action.

2017-03-28 Thread Alonzo Zhou 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57870/
---

(Updated 三月 28, 2017, 5:41 p.m.)


Review request for oozie.


Changes
---

1 Update CoordWfActionInfoXCommand : move some SQLs and JPA calls from 
execute() to loadState(). 
2 Change the method getWfActionByJobIdAndName in BaseJobServlet from abstract 
to protected.
could you please take a look? thanks!


Repository: oozie-git


Description
---

More detailed view of the coordinator’s history can be observed from 
perspective of  workflow action.


Diffs (updated)
-

  client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 8ddb1f8 
  core/src/main/java/org/apache/oozie/CoordinatorEngine.java 2f9f822 
  core/src/main/java/org/apache/oozie/ErrorCode.java b03ad06 
  
core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
 PRE-CREATION 
  core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java 03acbc1 
  core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java 9356768 
  core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java 6c30f5d 
  core/src/test/java/org/apache/oozie/client/TestOozieCLI.java 564db2a 
  
core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java
 PRE-CREATION 
  core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java 
4fc8653 
  core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java fb203a6 


Diff: https://reviews.apache.org/r/57870/diff/3/

Changes: https://reviews.apache.org/r/57870/diff/2-3/


Testing
---


Thanks,

Alonzo Zhou

Re: Review Request 57870: OOZIE-2827 More directly view of the coordinator’s history from perspective of workflow action.

2017-03-28 Thread Alonzo Zhou 


> On 三月 23, 2017, 11:35 a.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/servlet/V0JobServlet.java
> > Lines 280-285 (patched)
> > 
> >
> > Why not have this default implementation in `BaseJobServlet` instead?
> 
> Alonzo Zhou wrote:
> hi, i noticed that the methods in BaseJobServlet was defined as an 
> abstract method in addition to doGet and doPut. in order to maintain 
> consistency, then i defined getWfActionsInCoord as an abstract.
> 
> András Piros wrote:
> There are already non-abstract methods like `checkAuthorizationForApp()` 
> present in `BaseJobServlet`, so I wouldn't mind putting there a `protected` 
> method those implementation is the same for all children. Having duplicate 
> code is never OK - [DRY 
> principle](https://en.wikipedia.org/wiki/Don't_repeat_yourself).

Thanks for your advice, i'll fix it right away.  and I will remember that 
"Having duplicate code is never OK".


- Alonzo


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57870/#review169856
---


On 三月 28, 2017, 9:36 a.m., Alonzo Zhou wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57870/
> ---
> 
> (Updated 三月 28, 2017, 9:36 a.m.)
> 
> 
> Review request for oozie.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> More detailed view of the coordinator’s history can be observed from 
> perspective of  workflow action.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 
> 8ddb1f8 
>   core/src/main/java/org/apache/oozie/CoordinatorEngine.java 2f9f822 
>   core/src/main/java/org/apache/oozie/ErrorCode.java b03ad06 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
>  PRE-CREATION 
>   core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java 03acbc1 
>   core/src/main/java/org/apache/oozie/servlet/V0JobServlet.java d3b4689 
>   core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java 9356768 
>   core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java 6c30f5d 
>   core/src/test/java/org/apache/oozie/client/TestOozieCLI.java 564db2a 
>   
> core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java 
> 4fc8653 
>   core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java fb203a6 
> 
> 
> Diff: https://reviews.apache.org/r/57870/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alonzo Zhou
> 
>

Re: Review Request 57870: OOZIE-2827 More directly view of the coordinator’s history from perspective of workflow action.

2017-03-28 Thread András Piros 


> On March 23, 2017, 11:35 a.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
> > Lines 102-105 (patched)
> > 
> >
> > I'm wondering whether some of the SQLs and JPA calls of `execute()` 
> > couldn't be done here. `loadState()` is called only once, but `execute()` 
> > possibly many times.
> 
> Alonzo Zhou wrote:
> hi , if call XCommand.call(), loadState() and execute() will both 
> execute, so i think this doesnt much matter. did i think wrong?

You're right, these are meant to be called exactly the same times. 
Nevertheless, semantically it would be much nicer to use `loadState()` for - 
err, loading the state of the command :D


> On March 23, 2017, 11:35 a.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/servlet/V0JobServlet.java
> > Lines 280-285 (patched)
> > 
> >
> > Why not have this default implementation in `BaseJobServlet` instead?
> 
> Alonzo Zhou wrote:
> hi, i noticed that the methods in BaseJobServlet was defined as an 
> abstract method in addition to doGet and doPut. in order to maintain 
> consistency, then i defined getWfActionsInCoord as an abstract.

There are already non-abstract methods like `checkAuthorizationForApp()` 
present in `BaseJobServlet`, so I wouldn't mind putting there a `protected` 
method those implementation is the same for all children. Having duplicate code 
is never OK - [DRY 
principle](https://en.wikipedia.org/wiki/Don't_repeat_yourself).


> On March 23, 2017, 11:35 a.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java
> > Lines 1142-1146 (patched)
> > 
> >
> > Why not have this default implementation in `BaseJobServlet` instead?
> 
> Alonzo Zhou wrote:
> hi, i noticed that the methods in BaseJobServlet was defined as an 
> abstract method in addition to doGet and doPut. in order to maintain 
> consistency, then i defined getWfActionsInCoord as an abstract.

There are already non-abstract methods like `checkAuthorizationForApp()` 
present in `BaseJobServlet`, so I wouldn't mind putting there a `protected` 
method those implementation is the same for all children. Having duplicate code 
is never OK - [DRY 
principle](https://en.wikipedia.org/wiki/Don't_repeat_yourself).


- András


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57870/#review169856
---


On March 28, 2017, 9:36 a.m., Alonzo Zhou wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57870/
> ---
> 
> (Updated March 28, 2017, 9:36 a.m.)
> 
> 
> Review request for oozie.
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> More detailed view of the coordinator’s history can be observed from 
> perspective of  workflow action.
> 
> 
> Diffs
> -
> 
>   client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 
> 8ddb1f8 
>   core/src/main/java/org/apache/oozie/CoordinatorEngine.java 2f9f822 
>   core/src/main/java/org/apache/oozie/ErrorCode.java b03ad06 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
>  PRE-CREATION 
>   core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java 03acbc1 
>   core/src/main/java/org/apache/oozie/servlet/V0JobServlet.java d3b4689 
>   core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java 9356768 
>   core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java 6c30f5d 
>   core/src/test/java/org/apache/oozie/client/TestOozieCLI.java 564db2a 
>   
> core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java
>  PRE-CREATION 
>   
> core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java 
> 4fc8653 
>   core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java fb203a6 
> 
> 
> Diff: https://reviews.apache.org/r/57870/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alonzo Zhou
> 
>

Re: Review Request 57870: OOZIE-2827 More directly view of the coordinator’s history from perspective of workflow action.

2017-03-28 Thread Alonzo Zhou 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57870/
---

(Updated 三月 28, 2017, 9:36 a.m.)


Review request for oozie.


Repository: oozie-git


Description
---

More detailed view of the coordinator’s history can be observed from 
perspective of  workflow action.


Diffs (updated)
-

  client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 8ddb1f8 
  core/src/main/java/org/apache/oozie/CoordinatorEngine.java 2f9f822 
  core/src/main/java/org/apache/oozie/ErrorCode.java b03ad06 
  
core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
 PRE-CREATION 
  core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java 03acbc1 
  core/src/main/java/org/apache/oozie/servlet/V0JobServlet.java d3b4689 
  core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java 9356768 
  core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java 6c30f5d 
  core/src/test/java/org/apache/oozie/client/TestOozieCLI.java 564db2a 
  
core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java
 PRE-CREATION 
  core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java 
4fc8653 
  core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java fb203a6 


Diff: https://reviews.apache.org/r/57870/diff/2/

Changes: https://reviews.apache.org/r/57870/diff/1-2/


Testing
---


Thanks,

Alonzo Zhou

Re: Review Request 57870: OOZIE-2827 More directly view of the coordinator’s history from perspective of workflow action.

2017-03-28 Thread Alonzo Zhou 


> On 三月 23, 2017, 11:35 a.m., András Piros wrote:
> > Overall direction is OK, please see my comments.

Thanks for your reviewing man! I changed some following your advices, yet 
there're still some places that confuse me and I wrote them in comments.


> On 三月 23, 2017, 11:35 a.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
> > Lines 78 (patched)
> > 
> >
> > Logging a `WARN` here wouldn't hurt.
> > 
> > Why not return `Collections.emptyList()`?

wfAction is a element of List , in order to keep the length 
and order of the list returned. So add a null element.


> On 三月 23, 2017, 11:35 a.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
> > Lines 81 (patched)
> > 
> >
> > Logging a `WARN` here wouldn't hurt.
> > 
> > Why not return `Collections.emptyList()`?

wfAction is a element of List , in order to keep the length 
and order of the list returned. So add a null element.


> On 三月 23, 2017, 11:35 a.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
> > Lines 87 (patched)
> > 
> >
> > Explaining a bit more w/ a decent log message wouldn't hurt.

Yet after careful consideration, I still hold my point that this logging 
message is decent enough.


> On 三月 23, 2017, 11:35 a.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
> > Lines 102-105 (patched)
> > 
> >
> > I'm wondering whether some of the SQLs and JPA calls of `execute()` 
> > couldn't be done here. `loadState()` is called only once, but `execute()` 
> > possibly many times.

hi , if call XCommand.call(), loadState() and execute() will both execute, so i 
think this doesnt much matter. did i think wrong?


> On 三月 23, 2017, 11:35 a.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/servlet/V0JobServlet.java
> > Lines 280-285 (patched)
> > 
> >
> > Why not have this default implementation in `BaseJobServlet` instead?

hi, i noticed that the methods in BaseJobServlet was defined as an abstract 
method in addition to doGet and doPut. in order to maintain consistency, then i 
defined getWfActionsInCoord as an abstract.


> On 三月 23, 2017, 11:35 a.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java
> > Lines 1142-1146 (patched)
> > 
> >
> > Why not have this default implementation in `BaseJobServlet` instead?

hi, i noticed that the methods in BaseJobServlet was defined as an abstract 
method in addition to doGet and doPut. in order to maintain consistency, then i 
defined getWfActionsInCoord as an abstract.


> On 三月 23, 2017, 11:35 a.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java
> > Lines 401 (patched)
> > 
> >
> > Please make it more resilient to `NumberFormatException`.

hi, the method service(HttpServletRequest request, HttpServletResponse 
response) in JsonRestServlet catch the IllegalArgumentException and handle it ,
I'm wondering whether we should catch the NumberFormatException here?


> On 三月 23, 2017, 11:35 a.m., András Piros wrote:
> > core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java
> > Lines 408 (patched)
> > 
> >
> > Please make it more resilient to `NumberFormatException`.

hi, the method service(HttpServletRequest request, HttpServletResponse 
response) in JsonRestServlet catch the IllegalArgumentException and handle it ,
so I'm wondering whether we should catch the NumberFormatException here?


> On 三月 23, 2017, 11:35 a.m., András Piros wrote:
> > core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java
> > Lines 70 (patched)
> > 
> >
> > Why 2?

no particular reason, just a value I picked, as I think it doesn't matter anyway


> On 三月 23, 2017, 11:35 a.m., András Piros wrote:
> > core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java
> > Lines 261 (patched)
> > 
> >
> > Great idea to add functional tests as well :)
> > 
> > However, I'd focus more on the edge cases that can happen when calling 
> > `V2JobServlet` and weren't covered in other unit tests:

Re: Review Request 57870: OOZIE-2827 More directly view of the coordinator’s history from perspective of workflow action.

2017-03-23 Thread András Piros 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57870/#review169856
---



Overall direction is OK, please see my comments.


core/src/main/java/org/apache/oozie/CoordinatorEngine.java
Lines 1012 (patched)


I'd rename to `getWfActionByJobIdAndName()`.



core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
Lines 43 (patched)


There are no code paths where `offset` or `len` wouldn't be overwritten in 
a constructor. Please remove initialization here and make both variables 
`final`.



core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
Lines 47 (patched)


Why not have `50` as default `len`? Matching 
`CoordJobGetActionsSubsetJPAExecutor`, then.



core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
Lines 51 (patched)


Why not have `action.info` as constant? And a newline between `super()` and 
the rest would make the code more readable.



core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
Lines 67 (patched)


Logging an `ERROR` here wouldn't hurt.



core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
Lines 78 (patched)


Logging a `WARN` here wouldn't hurt.

Why not return `Collections.emptyList()`?



core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
Lines 81 (patched)


Logging a `WARN` here wouldn't hurt.

Why not return `Collections.emptyList()`?



core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
Lines 85 (patched)


Doing a `DEBUG` log here wouldn't hurt.



core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
Lines 87 (patched)


Explaining a bit more w/ a decent log message wouldn't hurt.



core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
Lines 88 (patched)


Why not return `Collections.emptyList()`?



core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
Lines 102-105 (patched)


I'm wondering whether some of the SQLs and JPA calls of `execute()` 
couldn't be done here. `loadState()` is called only once, but `execute()` 
possibly many times.



core/src/main/java/org/apache/oozie/servlet/V0JobServlet.java
Lines 280-285 (patched)


Why not have this default implementation in `BaseJobServlet` instead?



core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java
Lines 1142-1146 (patched)


Why not have this default implementation in `BaseJobServlet` instead?



core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java
Lines 401 (patched)


Please make it more resilient to `NumberFormatException`.



core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java
Lines 408 (patched)


Please make it more resilient to `NumberFormatException`.



core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java
Lines 411 (patched)


You can have it within `try` block.



core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java
Lines 42 (patched)


I'd also create more test scenarios here, like one testing boundaries for 
`offset` and `len`, and calling the parameterless constructor for both.



core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java
Lines 48-49 (patched)


Actually `LocalOozie.start()` already calls `Services.init()`, so no need 
to call that twice.



core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java
Lines 56 (patched)


Actually `LocalOozie.stop()` already calls `Services.destroy()`, so no need 
to do so twice.



core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java
Lines 62 (patched)


Maybe a message that

Re: Review Request 57870: OOZIE-2827 More directly view of the coordinator’s history from perspective of workflow action.

2017-03-23 Thread Alonzo Zhou 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57870/
---

(Updated 三月 23, 2017, 9:07 a.m.)


Review request for oozie.


Repository: oozie-git


Description (updated)
---

More detailed view of the coordinator’s history can be observed from 
perspective of  workflow action.


Diffs
-

  client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 4e38b4a 
  core/src/main/java/org/apache/oozie/CoordinatorEngine.java 2f9f822 
  
core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
 PRE-CREATION 
  core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java 03acbc1 
  core/src/main/java/org/apache/oozie/servlet/V0JobServlet.java d3b4689 
  core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java 9356768 
  core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java 6c30f5d 
  
core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java
 PRE-CREATION 
  core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java 
4fc8653 
  core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java fb203a6 


Diff: https://reviews.apache.org/r/57870/diff/1/


Testing
---


Thanks,

Alonzo Zhou

Review Request 57870: OOZIE-2827 More directly view of the coordinator’s history from perspective of workflow action.

2017-03-23 Thread Alonzo Zhou 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57870/
---

Review request for oozie.


Repository: oozie-git


Description
---

More directly view of the coordinator’s history from perspective of workflow 
action.


Diffs
-

  client/src/main/java/org/apache/oozie/client/rest/RestConstants.java 4e38b4a 
  core/src/main/java/org/apache/oozie/CoordinatorEngine.java 2f9f822 
  
core/src/main/java/org/apache/oozie/command/coord/CoordWfActionInfoXCommand.java
 PRE-CREATION 
  core/src/main/java/org/apache/oozie/servlet/BaseJobServlet.java 03acbc1 
  core/src/main/java/org/apache/oozie/servlet/V0JobServlet.java d3b4689 
  core/src/main/java/org/apache/oozie/servlet/V1JobServlet.java 9356768 
  core/src/main/java/org/apache/oozie/servlet/V2JobServlet.java 6c30f5d 
  
core/src/test/java/org/apache/oozie/command/coord/TestCoordWfActionInfoXCommand.java
 PRE-CREATION 
  core/src/test/java/org/apache/oozie/servlet/MockCoordinatorEngineService.java 
4fc8653 
  core/src/test/java/org/apache/oozie/servlet/TestV2JobServlet.java fb203a6 


Diff: https://reviews.apache.org/r/57870/diff/1/


Testing
---


Thanks,

Alonzo Zhou