Re: Review Request 24948: OOZIE-1940 StatusTransitService has race condition

2014-09-17 Thread Rohini Palaniswamy 

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



core/src/main/java/org/apache/oozie/CoordinatorJobBean.java


This is not from your patch. But )}) does not look right. Can you check 
which patch introduced this and fix this as well?


- Rohini Palaniswamy


On Sept. 17, 2014, 10:56 p.m., Purshotam Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24948/
> ---
> 
> (Updated Sept. 17, 2014, 10:56 p.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-1940
> https://issues.apache.org/jira/browse/OOZIE-1940
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> StatusTransitService has race condition
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/BundleActionBean.java 963497d 
>   core/src/main/java/org/apache/oozie/BundleJobBean.java 76e76b7 
>   core/src/main/java/org/apache/oozie/CoordinatorActionBean.java cc5596b 
>   core/src/main/java/org/apache/oozie/CoordinatorJobBean.java 71a9ab4 
>   core/src/main/java/org/apache/oozie/ErrorCode.java f9f88f4 
>   core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java 
> e69de29 
>   
> core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/BundleJobQueryExecutor.java 
> ce7473d 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/CoordActionQueryExecutor.java
>  0aee0e4 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetPendingActionsCountJPAExecutor.java
>  c96e5d9 
>   core/src/main/java/org/apache/oozie/executor/jpa/CoordJobQueryExecutor.java 
> 2c9e00e 
>   core/src/main/java/org/apache/oozie/service/StatusTransitService.java 
> 0d549a0 
>   
> core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetPendingActionsCountJPAExecutor.java
>  5b62fdf 
>   core/src/test/java/org/apache/oozie/service/TestStatusTransitService.java 
> 1f9e76a 
> 
> Diff: https://reviews.apache.org/r/24948/diff/
> 
> 
> Testing
> ---
> 
> UTC
> 
> 
> Thanks,
> 
> Purshotam Shah
> 
>

Re: Review Request 24948: OOZIE-1940 StatusTransitService has race condition

2014-09-17 Thread Rohini Palaniswamy 

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

Ship it!


Ship It!

- Rohini Palaniswamy


On Sept. 17, 2014, 10:56 p.m., Purshotam Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24948/
> ---
> 
> (Updated Sept. 17, 2014, 10:56 p.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-1940
> https://issues.apache.org/jira/browse/OOZIE-1940
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> StatusTransitService has race condition
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/BundleActionBean.java 963497d 
>   core/src/main/java/org/apache/oozie/BundleJobBean.java 76e76b7 
>   core/src/main/java/org/apache/oozie/CoordinatorActionBean.java cc5596b 
>   core/src/main/java/org/apache/oozie/CoordinatorJobBean.java 71a9ab4 
>   core/src/main/java/org/apache/oozie/ErrorCode.java f9f88f4 
>   core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java 
> e69de29 
>   
> core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/BundleJobQueryExecutor.java 
> ce7473d 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/CoordActionQueryExecutor.java
>  0aee0e4 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetPendingActionsCountJPAExecutor.java
>  c96e5d9 
>   core/src/main/java/org/apache/oozie/executor/jpa/CoordJobQueryExecutor.java 
> 2c9e00e 
>   core/src/main/java/org/apache/oozie/service/StatusTransitService.java 
> 0d549a0 
>   
> core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetPendingActionsCountJPAExecutor.java
>  5b62fdf 
>   core/src/test/java/org/apache/oozie/service/TestStatusTransitService.java 
> 1f9e76a 
> 
> Diff: https://reviews.apache.org/r/24948/diff/
> 
> 
> Testing
> ---
> 
> UTC
> 
> 
> Thanks,
> 
> Purshotam Shah
> 
>

Re: Review Request 24948: OOZIE-1940 StatusTransitService has race condition

2014-09-17 Thread Purshotam Shah 

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

(Updated Sept. 17, 2014, 10:56 p.m.)


Review request for oozie.


Bugs: OOZIE-1940
https://issues.apache.org/jira/browse/OOZIE-1940


Repository: oozie-git


Description
---

StatusTransitService has race condition


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/BundleActionBean.java 963497d 
  core/src/main/java/org/apache/oozie/BundleJobBean.java 76e76b7 
  core/src/main/java/org/apache/oozie/CoordinatorActionBean.java cc5596b 
  core/src/main/java/org/apache/oozie/CoordinatorJobBean.java 71a9ab4 
  core/src/main/java/org/apache/oozie/ErrorCode.java f9f88f4 
  core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java 
e69de29 
  
core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java
 e69de29 
  
core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java
 e69de29 
  core/src/main/java/org/apache/oozie/executor/jpa/BundleJobQueryExecutor.java 
ce7473d 
  
core/src/main/java/org/apache/oozie/executor/jpa/CoordActionQueryExecutor.java 
0aee0e4 
  
core/src/main/java/org/apache/oozie/executor/jpa/CoordJobGetPendingActionsCountJPAExecutor.java
 c96e5d9 
  core/src/main/java/org/apache/oozie/executor/jpa/CoordJobQueryExecutor.java 
2c9e00e 
  core/src/main/java/org/apache/oozie/service/StatusTransitService.java 0d549a0 
  
core/src/test/java/org/apache/oozie/executor/jpa/TestCoordJobGetPendingActionsCountJPAExecutor.java
 5b62fdf 
  core/src/test/java/org/apache/oozie/service/TestStatusTransitService.java 
1f9e76a 

Diff: https://reviews.apache.org/r/24948/diff/


Testing
---

UTC


Thanks,

Purshotam Shah

Re: Review Request 24948: OOZIE-1940 StatusTransitService has race condition

2014-09-12 Thread Rohini Palaniswamy 

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


StatusTransitXCommand abstract class defined methods in the order in which they 
are called. Please order the methods in the implementing commands same as the 
abstract class StatusTransitXCommand for easy code readability.


core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java


Change message as below to make it more clear when reading the logs. Also 
change error code to E1320

Bundle action coordId is null and bundle job is already in killed state. 
Ignoring.



core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java


complete the javadoc. Else precommit will show this as error



core/src/main/java/org/apache/oozie/service/StatusTransitService.java


Can you delete this class?


- Rohini Palaniswamy


On Sept. 11, 2014, 1:42 a.m., Purshotam Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24948/
> ---
> 
> (Updated Sept. 11, 2014, 1:42 a.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-1940
> https://issues.apache.org/jira/browse/OOZIE-1940
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> StatusTransitService has race condition
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/BundleActionBean.java 5d85a4d 
>   core/src/main/java/org/apache/oozie/BundleJobBean.java 0f1670a 
>   core/src/main/java/org/apache/oozie/CoordinatorActionBean.java 795bf63 
>   core/src/main/java/org/apache/oozie/CoordinatorJobBean.java 8fd53f1 
>   core/src/main/java/org/apache/oozie/ErrorCode.java 88a2c67 
>   core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java 
> e69de29 
>   
> core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/BundleJobQueryExecutor.java 
> 36cd968 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/CoordActionQueryExecutor.java
>  3008393 
>   core/src/main/java/org/apache/oozie/executor/jpa/CoordJobQueryExecutor.java 
> 04e6e29 
>   core/src/main/java/org/apache/oozie/service/StatusTransitService.java 
> 21ac25f 
>   core/src/test/java/org/apache/oozie/service/TestStatusTransitService.java 
> bb99138 
> 
> Diff: https://reviews.apache.org/r/24948/diff/
> 
> 
> Testing
> ---
> 
> UTC
> 
> 
> Thanks,
> 
> Purshotam Shah
> 
>

Re: Review Request 24948: OOZIE-1940 StatusTransitService has race condition

2014-09-10 Thread Purshotam Shah 

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

(Updated Sept. 11, 2014, 1:42 a.m.)


Review request for oozie.


Bugs: OOZIE-1940
https://issues.apache.org/jira/browse/OOZIE-1940


Repository: oozie-git


Description
---

StatusTransitService has race condition


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/BundleActionBean.java 5d85a4d 
  core/src/main/java/org/apache/oozie/BundleJobBean.java 0f1670a 
  core/src/main/java/org/apache/oozie/CoordinatorActionBean.java 795bf63 
  core/src/main/java/org/apache/oozie/CoordinatorJobBean.java 8fd53f1 
  core/src/main/java/org/apache/oozie/ErrorCode.java 88a2c67 
  core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java 
e69de29 
  
core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java
 e69de29 
  
core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java
 e69de29 
  core/src/main/java/org/apache/oozie/executor/jpa/BundleJobQueryExecutor.java 
36cd968 
  
core/src/main/java/org/apache/oozie/executor/jpa/CoordActionQueryExecutor.java 
3008393 
  core/src/main/java/org/apache/oozie/executor/jpa/CoordJobQueryExecutor.java 
04e6e29 
  core/src/main/java/org/apache/oozie/service/StatusTransitService.java 21ac25f 
  core/src/test/java/org/apache/oozie/service/TestStatusTransitService.java 
bb99138 

Diff: https://reviews.apache.org/r/24948/diff/


Testing
---

UTC


Thanks,

Purshotam Shah

Re: Review Request 24948: OOZIE-1940 StatusTransitService has race condition

2014-09-10 Thread Rohini Palaniswamy 

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



core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java


update only if something has changed



core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java


Remove saveToDB option.



core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java


This method is not used anywhere



core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java


hasTerminatedActions() - Same as bundle



core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java


Should not be updating database in getJobStatus. Already being done in the 
update method. Remove this from here


- Rohini Palaniswamy


On Sept. 8, 2014, 9:15 p.m., Purshotam Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24948/
> ---
> 
> (Updated Sept. 8, 2014, 9:15 p.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-1940
> https://issues.apache.org/jira/browse/OOZIE-1940
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> StatusTransitService has race condition
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/BundleActionBean.java 5d85a4d 
>   core/src/main/java/org/apache/oozie/BundleJobBean.java 0f1670a 
>   core/src/main/java/org/apache/oozie/CoordinatorActionBean.java 795bf63 
>   core/src/main/java/org/apache/oozie/CoordinatorJobBean.java 8fd53f1 
>   core/src/main/java/org/apache/oozie/ErrorCode.java 88a2c67 
>   core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java 
> e69de29 
>   
> core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/BundleJobQueryExecutor.java 
> 36cd968 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/CoordActionQueryExecutor.java
>  3008393 
>   core/src/main/java/org/apache/oozie/executor/jpa/CoordJobQueryExecutor.java 
> 04e6e29 
>   core/src/main/java/org/apache/oozie/service/StatusTransitService.java 
> 21ac25f 
>   core/src/test/java/org/apache/oozie/service/TestStatusTransitService.java 
> bb99138 
> 
> Diff: https://reviews.apache.org/r/24948/diff/
> 
> 
> Testing
> ---
> 
> UTC
> 
> 
> Thanks,
> 
> Purshotam Shah
> 
>

Re: Review Request 24948: OOZIE-1940 StatusTransitService has race condition

2014-09-10 Thread Rohini Palaniswamy 

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



core/src/main/java/org/apache/oozie/BundleJobBean.java


1) GET_BUNDLE_IDS_BY_LAST_MODIFIED_TIME -> GET_BUNDLE_IDS_FOR_STATUS_TRANSIT

2) Either A or a through out. If any of the supported databases is 
case-sensitive it will be a problem.

3) select distinct  instead of group by at the end

4) Nitpick. spaces before and after = in w.id=a.bundleId



core/src/main/java/org/apache/oozie/CoordinatorJobBean.java


1) GET_COORD_IDS_BY_LAST_MODIFIED_TIME -> GET_COORD_IDS_FOR_STATUS_TRANSIT

2) distinct instead of group by

3) join clause w.id = a.jobId is missing.



core/src/main/java/org/apache/oozie/ErrorCode.java


Coord status transit error.



core/src/main/java/org/apache/oozie/ErrorCode.java


Bundle status transit error



core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java


0L



core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java


jobStatus



core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java


Current code will update status even if there is no change



core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java


Though this method is very readable, understanding the full status transit 
logic is very hard and requires lot of hops between methods to read. Requires 
rework.



core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java


&& bundleJob.getStatus() != Job.Status.KILLED needs to be removed



core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java


Remove variable and log directly



core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java


Can we get delete CoordJobGetPendingActionsCountJPAExecutor and change to 
CoordJobQueryExecutor.getSingleValue. Refer to 
SLASummaryQueryExecutor.getSingleValue().



core/src/main/java/org/apache/oozie/service/StatusTransitService.java


Get rid of these. Not actually used



core/src/main/java/org/apache/oozie/service/StatusTransitService.java


Get rid of the hashset



core/src/main/java/org/apache/oozie/service/StatusTransitService.java


merge this method into bundleTransit



core/src/main/java/org/apache/oozie/service/StatusTransitService.java


merge this method into coordTransit



core/src/test/java/org/apache/oozie/service/TestStatusTransitService.java


services = new Services();
services.init();

to avoid leaking services



core/src/test/java/org/apache/oozie/service/TestStatusTransitService.java


JobLock. Why do you need a separate thread and class for acquiring and 
releasing locks?

Can't you just do 

LockToken lock = 
Services.get().get(MemoryLocksService.class).getWriteLock(jobId, 0);
.
Perform operations
.
lock.release();

in the test methods


- Rohini Palaniswamy


On Sept. 8, 2014, 9:15 p.m., Purshotam Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24948/
> ---
> 
> (Updated Sept. 8, 2014, 9:15 p.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-1940
> https://issues.apache.org/jira/browse/OOZIE-1940
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> StatusTransitService has race condition
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/BundleActionBean.java 5d85a4d 
>   core/src/main/java/org/apache/oozie/BundleJobBean.java 0f1670a 
>   core/src/main/java/org/apache/oozie/CoordinatorActionBean.java 795bf63 
>   core/src/main/java/org/apache/oozie/CoordinatorJobBean.java 8fd53f1 
>   core/src/main/java/org/apache/oozie/ErrorCode.java 88a2c67 
>   core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java 
> e69de29 
>

Re: Review Request 24948: OOZIE-1940 StatusTransitService has race condition

2014-09-09 Thread Mona Chitnis 

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

Ship it!


Looks good now. Thanks for the 2 clarifications above.

- Mona Chitnis


On Sept. 8, 2014, 9:15 p.m., Purshotam Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24948/
> ---
> 
> (Updated Sept. 8, 2014, 9:15 p.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-1940
> https://issues.apache.org/jira/browse/OOZIE-1940
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> StatusTransitService has race condition
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/BundleActionBean.java 5d85a4d 
>   core/src/main/java/org/apache/oozie/BundleJobBean.java 0f1670a 
>   core/src/main/java/org/apache/oozie/CoordinatorActionBean.java 795bf63 
>   core/src/main/java/org/apache/oozie/CoordinatorJobBean.java 8fd53f1 
>   core/src/main/java/org/apache/oozie/ErrorCode.java 88a2c67 
>   core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java 
> e69de29 
>   
> core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/BundleJobQueryExecutor.java 
> 36cd968 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/CoordActionQueryExecutor.java
>  3008393 
>   core/src/main/java/org/apache/oozie/executor/jpa/CoordJobQueryExecutor.java 
> 04e6e29 
>   core/src/main/java/org/apache/oozie/service/StatusTransitService.java 
> 21ac25f 
>   core/src/test/java/org/apache/oozie/service/TestStatusTransitService.java 
> bb99138 
> 
> Diff: https://reviews.apache.org/r/24948/diff/
> 
> 
> Testing
> ---
> 
> UTC
> 
> 
> Thanks,
> 
> Purshotam Shah
> 
>

Re: Review Request 24948: OOZIE-1940 StatusTransitService has race condition

2014-09-09 Thread Purshotam Shah 


> On Sept. 9, 2014, 3:12 p.m., Mona Chitnis wrote:
> > core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java,
> >  line 81
> > 
> >
> > But this is executed only if condition bAction.getCoordId() == null. So 
> > the case you mention will not occur

It will. "condition bAction.getCoordId() == null" means that one of bundle 
coord submit has failed and we need to kill bundle job.


> On Sept. 9, 2014, 3:12 p.m., Mona Chitnis wrote:
> > core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java,
> >  line 168
> > 
> >
> > Then I think we should 'skip' loading in SKIPPED actions from DB. The 
> > idea of skipped is that its outcome would not make any difference. So if 
> > all other actions are terminal, you will mark parent coord/bundle as 
> > terminal. if some are non-terminal, it wont be. ok to optimize this from 
> > earlier code

It make difference.
Assume there are 3 coord actions. 
1. failed.
2. Skipped
3. Skipped.

Current behavior, final status will be DONEWITHERROR.
If I skip "Skipped" action then, final status will be failed. Same with killed.

I think it's ok to not change existing behavior.


- Purshotam


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


On Sept. 8, 2014, 9:15 p.m., Purshotam Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24948/
> ---
> 
> (Updated Sept. 8, 2014, 9:15 p.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-1940
> https://issues.apache.org/jira/browse/OOZIE-1940
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> StatusTransitService has race condition
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/BundleActionBean.java 5d85a4d 
>   core/src/main/java/org/apache/oozie/BundleJobBean.java 0f1670a 
>   core/src/main/java/org/apache/oozie/CoordinatorActionBean.java 795bf63 
>   core/src/main/java/org/apache/oozie/CoordinatorJobBean.java 8fd53f1 
>   core/src/main/java/org/apache/oozie/ErrorCode.java 88a2c67 
>   core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java 
> e69de29 
>   
> core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/BundleJobQueryExecutor.java 
> 36cd968 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/CoordActionQueryExecutor.java
>  3008393 
>   core/src/main/java/org/apache/oozie/executor/jpa/CoordJobQueryExecutor.java 
> 04e6e29 
>   core/src/main/java/org/apache/oozie/service/StatusTransitService.java 
> 21ac25f 
>   core/src/test/java/org/apache/oozie/service/TestStatusTransitService.java 
> bb99138 
> 
> Diff: https://reviews.apache.org/r/24948/diff/
> 
> 
> Testing
> ---
> 
> UTC
> 
> 
> Thanks,
> 
> Purshotam Shah
> 
>

Re: Review Request 24948: OOZIE-1940 StatusTransitService has race condition

2014-09-09 Thread Mona Chitnis 


> On Sept. 4, 2014, 8:55 p.m., Mona Chitnis wrote:
> > core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java,
> >  line 177
> > 
> >
> > related question, is this situation possible? - job status is PAUSED || 
> > PWE, and bundle action status is RWE?
> 
> Purshotam Shah wrote:
> Yes, BundlePauseXCommand only set bundle status to pause. Bundle status 
> can still be in running state.

Ok. I just checked that BundlePauseXCommand and CoordPauseXCommand have empty 
implementations of pauseChildren()
@Override
public void pauseChildren() throws CommandException {
// TODO - need revisit when revisiting coord job status redesign;

}


- Mona


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


On Sept. 8, 2014, 9:15 p.m., Purshotam Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24948/
> ---
> 
> (Updated Sept. 8, 2014, 9:15 p.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-1940
> https://issues.apache.org/jira/browse/OOZIE-1940
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> StatusTransitService has race condition
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/BundleActionBean.java 5d85a4d 
>   core/src/main/java/org/apache/oozie/BundleJobBean.java 0f1670a 
>   core/src/main/java/org/apache/oozie/CoordinatorActionBean.java 795bf63 
>   core/src/main/java/org/apache/oozie/CoordinatorJobBean.java 8fd53f1 
>   core/src/main/java/org/apache/oozie/ErrorCode.java 88a2c67 
>   core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java 
> e69de29 
>   
> core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/BundleJobQueryExecutor.java 
> 36cd968 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/CoordActionQueryExecutor.java
>  3008393 
>   core/src/main/java/org/apache/oozie/executor/jpa/CoordJobQueryExecutor.java 
> 04e6e29 
>   core/src/main/java/org/apache/oozie/service/StatusTransitService.java 
> 21ac25f 
>   core/src/test/java/org/apache/oozie/service/TestStatusTransitService.java 
> bb99138 
> 
> Diff: https://reviews.apache.org/r/24948/diff/
> 
> 
> Testing
> ---
> 
> UTC
> 
> 
> Thanks,
> 
> Purshotam Shah
> 
>

Re: Review Request 24948: OOZIE-1940 StatusTransitService has race condition

2014-09-09 Thread Mona Chitnis 

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



core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java


But this is executed only if condition bAction.getCoordId() == null. So the 
case you mention will not occur



core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java


Then I think we should 'skip' loading in SKIPPED actions from DB. The idea 
of skipped is that its outcome would not make any difference. So if all other 
actions are terminal, you will mark parent coord/bundle as terminal. if some 
are non-terminal, it wont be. ok to optimize this from earlier code


- Mona Chitnis


On Sept. 8, 2014, 9:15 p.m., Purshotam Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24948/
> ---
> 
> (Updated Sept. 8, 2014, 9:15 p.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-1940
> https://issues.apache.org/jira/browse/OOZIE-1940
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> StatusTransitService has race condition
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/BundleActionBean.java 5d85a4d 
>   core/src/main/java/org/apache/oozie/BundleJobBean.java 0f1670a 
>   core/src/main/java/org/apache/oozie/CoordinatorActionBean.java 795bf63 
>   core/src/main/java/org/apache/oozie/CoordinatorJobBean.java 8fd53f1 
>   core/src/main/java/org/apache/oozie/ErrorCode.java 88a2c67 
>   core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java 
> e69de29 
>   
> core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/BundleJobQueryExecutor.java 
> 36cd968 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/CoordActionQueryExecutor.java
>  3008393 
>   core/src/main/java/org/apache/oozie/executor/jpa/CoordJobQueryExecutor.java 
> 04e6e29 
>   core/src/main/java/org/apache/oozie/service/StatusTransitService.java 
> 21ac25f 
>   core/src/test/java/org/apache/oozie/service/TestStatusTransitService.java 
> bb99138 
> 
> Diff: https://reviews.apache.org/r/24948/diff/
> 
> 
> Testing
> ---
> 
> UTC
> 
> 
> Thanks,
> 
> Purshotam Shah
> 
>

Re: Review Request 24948: OOZIE-1940 StatusTransitService has race condition

2014-09-08 Thread Purshotam Shah 

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

(Updated Sept. 8, 2014, 9:15 p.m.)


Review request for oozie.


Bugs: OOZIE-1940
https://issues.apache.org/jira/browse/OOZIE-1940


Repository: oozie-git


Description
---

StatusTransitService has race condition


Diffs (updated)
-

  core/src/main/java/org/apache/oozie/BundleActionBean.java 5d85a4d 
  core/src/main/java/org/apache/oozie/BundleJobBean.java 0f1670a 
  core/src/main/java/org/apache/oozie/CoordinatorActionBean.java 795bf63 
  core/src/main/java/org/apache/oozie/CoordinatorJobBean.java 8fd53f1 
  core/src/main/java/org/apache/oozie/ErrorCode.java 88a2c67 
  core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java 
e69de29 
  
core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java
 e69de29 
  
core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java
 e69de29 
  core/src/main/java/org/apache/oozie/executor/jpa/BundleJobQueryExecutor.java 
36cd968 
  
core/src/main/java/org/apache/oozie/executor/jpa/CoordActionQueryExecutor.java 
3008393 
  core/src/main/java/org/apache/oozie/executor/jpa/CoordJobQueryExecutor.java 
04e6e29 
  core/src/main/java/org/apache/oozie/service/StatusTransitService.java 21ac25f 
  core/src/test/java/org/apache/oozie/service/TestStatusTransitService.java 
bb99138 

Diff: https://reviews.apache.org/r/24948/diff/


Testing
---

UTC


Thanks,

Purshotam Shah

Re: Review Request 24948: OOZIE-1940 StatusTransitService has race condition

2014-09-08 Thread Purshotam Shah 


> On Sept. 4, 2014, 8:55 p.m., Mona Chitnis wrote:
> > core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java,
> >  line 81
> > 
> >
> > do we need to execute this synchronously? None of the counts here are 
> > affected by the outcome of this command. Can queue it to release lock faster

We need to kill bundle and coord asap. There may be chance the coord action 
have started and might be some processing. Since it's a faulty bundle, we don't 
want to start any processing.
This will be fixed in OOZIE-1863.


> On Sept. 4, 2014, 8:55 p.m., Mona Chitnis wrote:
> > core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java,
> >  line 177
> > 
> >
> > related question, is this situation possible? - job status is PAUSED || 
> > PWE, and bundle action status is RWE?

Yes, BundlePauseXCommand only set bundle status to pause. Bundle status can 
still be in running state.


> On Sept. 4, 2014, 8:55 p.m., Mona Chitnis wrote:
> > core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java,
> >  line 132
> > 
> >
> > same comment as BundleStatusTransitX, just make sure whether 
> > SuspendedWithError is the right status here

Yes, we first check the terminal case. SUSPENDED  check will come after 
terminal check.


> On Sept. 4, 2014, 8:55 p.m., Mona Chitnis wrote:
> > core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java,
> >  line 168
> > 
> >
> > why dont we filter out the SKIPPED actions altogether for status 
> > transit processing?

We check if all action is in terminal state then, we mark job as terminated. 
SKIPPED is also a terminal state. Previous StatusTransitService does same way.


> On Sept. 4, 2014, 8:55 p.m., Mona Chitnis wrote:
> > core/src/main/java/org/apache/oozie/service/StatusTransitService.java, line 
> > 160
> > 
> >
> > some form of batching done right away would be good, so we can execute 
> > bundle/coord update queries on multiple bundles/coords in a batched execute 
> > query. However then we wont be able to utilize the locking per job to have 
> > strong consistency. I guess ok to defer this until we have a better idea

Yes, previous StatusTransitService does the same.

Lets create a different JIRA for batching.


> On Sept. 4, 2014, 8:55 p.m., Mona Chitnis wrote:
> > core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java,
> >  line 248
> > 
> >
> > is this a change from current behavior? a mix of suspended, failed, 
> > killed, DWE, SPE = SPE? I'm not sure. Sounds reasonable to me but need to 
> > check if this could potentially be confusing to anyone

There is no change in behaviour. Previous StatusTransitService has same 
condition.


> On Sept. 4, 2014, 8:55 p.m., Mona Chitnis wrote:
> > core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java,
> >  line 267
> > 
> >
> > I dont understand this. Are the other bundle actions in Running? Then 
> > why is status Prep and not Running?

It should be PrepRunning state.


- Purshotam


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


On Aug. 26, 2014, 12:30 a.m., Purshotam Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24948/
> ---
> 
> (Updated Aug. 26, 2014, 12:30 a.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-1940
> https://issues.apache.org/jira/browse/OOZIE-1940
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> StatusTransitService has race condition
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/BundleActionBean.java 5d85a4d 
>   core/src/main/java/org/apache/oozie/BundleJobBean.java 0f1670a 
>   core/src/main/java/org/apache/oozie/CoordinatorActionBean.java 795bf63 
>   core/src/main/java/org/apache/oozie/CoordinatorJobBean.java 8fd53f1 
>   core/src/main/java/org/apache/oozie/ErrorCode.java 88a2c67 
>   core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java 
> e69de29 
>   
> core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/command/coo

Re: Review Request 24948: OOZIE-1940 StatusTransitService has race condition

2014-09-08 Thread Purshotam Shah 

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



core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java


There is no change in behaviour. Previous StatusTransitService has same 
condition.



core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java


It should be PrepRunning state.



core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java


Yes, we first check the terminal case. SUSPENDED  check will come after 
terminal check.



core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java


We check if all action is in terminal state then, we mark job as 
terminated. SKIPPED is also a terminal state.



core/src/main/java/org/apache/oozie/service/StatusTransitService.java


Yes, previous StatusTransitService does the same.

Lets create a different JIRA for batching.


- Purshotam Shah


On Aug. 26, 2014, 12:30 a.m., Purshotam Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24948/
> ---
> 
> (Updated Aug. 26, 2014, 12:30 a.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-1940
> https://issues.apache.org/jira/browse/OOZIE-1940
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> StatusTransitService has race condition
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/BundleActionBean.java 5d85a4d 
>   core/src/main/java/org/apache/oozie/BundleJobBean.java 0f1670a 
>   core/src/main/java/org/apache/oozie/CoordinatorActionBean.java 795bf63 
>   core/src/main/java/org/apache/oozie/CoordinatorJobBean.java 8fd53f1 
>   core/src/main/java/org/apache/oozie/ErrorCode.java 88a2c67 
>   core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java 
> e69de29 
>   
> core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/BundleJobQueryExecutor.java 
> 36cd968 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/CoordActionQueryExecutor.java
>  3008393 
>   core/src/main/java/org/apache/oozie/executor/jpa/CoordJobQueryExecutor.java 
> 04e6e29 
>   core/src/main/java/org/apache/oozie/service/StatusTransitService.java 
> 21ac25f 
>   core/src/test/java/org/apache/oozie/service/TestStatusTransitService.java 
> bb99138 
> 
> Diff: https://reviews.apache.org/r/24948/diff/
> 
> 
> Testing
> ---
> 
> UTC
> 
> 
> Thanks,
> 
> Purshotam Shah
> 
>

Re: Review Request 24948: OOZIE-1940 StatusTransitService has race condition

2014-09-04 Thread Mona Chitnis 

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



core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java


do we need to execute this synchronously? None of the counts here are 
affected by the outcome of this command. Can queue it to release lock faster



core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java


related question, is this situation possible? - job status is PAUSED || 
PWE, and bundle action status is RWE?



core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java


is this a change from current behavior? a mix of suspended, failed, killed, 
DWE, SPE = SPE? I'm not sure. Sounds reasonable to me but need to check if this 
could potentially be confusing to anyone



core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java


I dont understand this. Are the other bundle actions in Running? Then why 
is status Prep and not Running?



core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java


why is getPrepStatus calling getRunningStatus?



core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java


same comment as BundleStatusTransitX, just make sure whether 
SuspendedWithError is the right status here



core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java


why dont we filter out the SKIPPED actions altogether for status transit 
processing?



core/src/main/java/org/apache/oozie/service/StatusTransitService.java


some form of batching done right away would be good, so we can execute 
bundle/coord update queries on multiple bundles/coords in a batched execute 
query. However then we wont be able to utilize the locking per job to have 
strong consistency. I guess ok to defer this until we have a better idea


- Mona Chitnis


On Aug. 26, 2014, 12:30 a.m., Purshotam Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24948/
> ---
> 
> (Updated Aug. 26, 2014, 12:30 a.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-1940
> https://issues.apache.org/jira/browse/OOZIE-1940
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> StatusTransitService has race condition
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/BundleActionBean.java 5d85a4d 
>   core/src/main/java/org/apache/oozie/BundleJobBean.java 0f1670a 
>   core/src/main/java/org/apache/oozie/CoordinatorActionBean.java 795bf63 
>   core/src/main/java/org/apache/oozie/CoordinatorJobBean.java 8fd53f1 
>   core/src/main/java/org/apache/oozie/ErrorCode.java 88a2c67 
>   core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java 
> e69de29 
>   
> core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/BundleJobQueryExecutor.java 
> 36cd968 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/CoordActionQueryExecutor.java
>  3008393 
>   core/src/main/java/org/apache/oozie/executor/jpa/CoordJobQueryExecutor.java 
> 04e6e29 
>   core/src/main/java/org/apache/oozie/service/StatusTransitService.java 
> 21ac25f 
>   core/src/test/java/org/apache/oozie/service/TestStatusTransitService.java 
> bb99138 
> 
> Diff: https://reviews.apache.org/r/24948/diff/
> 
> 
> Testing
> ---
> 
> UTC
> 
> 
> Thanks,
> 
> Purshotam Shah
> 
>

Re: Review Request 24948: OOZIE-1940 StatusTransitService has race condition

2014-08-27 Thread Mona Chitnis 

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


this is good cleanup and refactoring. Did cursory review to understand 
structural changes. Will review more carefully for any bugs introduced later 
today


core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java


if bundleActionStatus map has some action as RUNNINGWITHERROR, why are we 
setting bundle job to PAUSED?



core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java


typo in bottom



core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java


same typo


- Mona Chitnis


On Aug. 26, 2014, 12:30 a.m., Purshotam Shah wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24948/
> ---
> 
> (Updated Aug. 26, 2014, 12:30 a.m.)
> 
> 
> Review request for oozie.
> 
> 
> Bugs: OOZIE-1940
> https://issues.apache.org/jira/browse/OOZIE-1940
> 
> 
> Repository: oozie-git
> 
> 
> Description
> ---
> 
> StatusTransitService has race condition
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/oozie/BundleActionBean.java 5d85a4d 
>   core/src/main/java/org/apache/oozie/BundleJobBean.java 0f1670a 
>   core/src/main/java/org/apache/oozie/CoordinatorActionBean.java 795bf63 
>   core/src/main/java/org/apache/oozie/CoordinatorJobBean.java 8fd53f1 
>   core/src/main/java/org/apache/oozie/ErrorCode.java 88a2c67 
>   core/src/main/java/org/apache/oozie/command/StatusTransitXCommand.java 
> e69de29 
>   
> core/src/main/java/org/apache/oozie/command/bundle/BundleStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/command/coord/CoordStatusTransitXCommand.java
>  e69de29 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/BundleJobQueryExecutor.java 
> 36cd968 
>   
> core/src/main/java/org/apache/oozie/executor/jpa/CoordActionQueryExecutor.java
>  3008393 
>   core/src/main/java/org/apache/oozie/executor/jpa/CoordJobQueryExecutor.java 
> 04e6e29 
>   core/src/main/java/org/apache/oozie/service/StatusTransitService.java 
> 21ac25f 
>   core/src/test/java/org/apache/oozie/service/TestStatusTransitService.java 
> bb99138 
> 
> Diff: https://reviews.apache.org/r/24948/diff/
> 
> 
> Testing
> ---
> 
> UTC
> 
> 
> Thanks,
> 
> Purshotam Shah
> 
>