Re: Review Request 25300: Add a state machine to react to job update status changes.

[Maxim Khutornenko]

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

Ship it!


Ship It!

- Maxim Khutornenko


On Sept. 3, 2014, 4:02 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25300/
> ---
> 
> (Updated Sept. 3, 2014, 4:02 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This will be used by the job updater to gate insertion of JobUpdateEvents.
> 
> I chose not to use StateMachine here as it actually added complexity - in 
> this case we don't benefit from transition callbacks, and the result is based 
> only on the target state (rather than the transition).
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/25300/diff/
> 
> 
> Testing
> ---
> 
> `./gradlew build -Pq`
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 25300: Add a state machine to react to job update status changes.

[Maxim Khutornenko]

> On Sept. 3, 2014, 5:33 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java,
> >  lines 74-76
> > 
> >
> > This will also throw for in-place transitions (e.g. ROLL_FORWARD_PAUSED 
> > -> ROLL_FORWARD_PAUSED) making it harder to implement idempotent thrift 
> > APIs (i.e. pause/resume/abort). Would it make sense to alter 
> > ALLOWED_TRANSITIONS with same state no-op transitions or throw here only if 
> > from != to?
> 
> Bill Farner wrote:
> I didn't see much value in supporting no-op transitions.  The caller can 
> suppress that easily enough, so i chose to be more restrictive here.
> 
> Maxim Khutornenko wrote:
> I guess I don't see how the caller would be able to set apart illegal 
> state transition (valid error) from the same state one. Are you suggesting 
> querying the state before trying to call into the controller?
> 
> Bill Farner wrote:
> I'm suggesting that the caller avoid invoking transition(a, b) where 
> a==b.  If you'd prefer, i can allow that and return a NO_OP action, but all 
> that really does is move code around.  I don't have a strong opinion, neither 
> seems more attractive:
> 
> ```
> if (current != next) {
>   switch (transition(current, next)) {
> case STOP_WATCHING:
>   ...
>   }
> }
> ```
> 
> ```
> switch (transition(current, next)) {
>   case NO_OP:
> break;
>   case STOP_WATCHING:
> ...
> }
> ```
> 
> Bill Farner wrote:
> Just realized this may be a case of different definitions of _caller_.  
> My _caller_ is the next level up in the call stack, sounds yours is an HTTP 
> API consumer.

Yes, the missing context really helps here. I was assuming this would be 
directly exposed to the scheduler API. The first example makes sense (i.e. 
no-op seems redundant). Thanks for explaining.


- Maxim


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


On Sept. 3, 2014, 4:02 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25300/
> ---
> 
> (Updated Sept. 3, 2014, 4:02 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This will be used by the job updater to gate insertion of JobUpdateEvents.
> 
> I chose not to use StateMachine here as it actually added complexity - in 
> this case we don't benefit from transition callbacks, and the result is based 
> only on the target state (rather than the transition).
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/25300/diff/
> 
> 
> Testing
> ---
> 
> `./gradlew build -Pq`
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 25300: Add a state machine to react to job update status changes.

[Bill Farner]

> On Sept. 3, 2014, 5:33 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java,
> >  lines 74-76
> > 
> >
> > This will also throw for in-place transitions (e.g. ROLL_FORWARD_PAUSED 
> > -> ROLL_FORWARD_PAUSED) making it harder to implement idempotent thrift 
> > APIs (i.e. pause/resume/abort). Would it make sense to alter 
> > ALLOWED_TRANSITIONS with same state no-op transitions or throw here only if 
> > from != to?
> 
> Bill Farner wrote:
> I didn't see much value in supporting no-op transitions.  The caller can 
> suppress that easily enough, so i chose to be more restrictive here.
> 
> Maxim Khutornenko wrote:
> I guess I don't see how the caller would be able to set apart illegal 
> state transition (valid error) from the same state one. Are you suggesting 
> querying the state before trying to call into the controller?
> 
> Bill Farner wrote:
> I'm suggesting that the caller avoid invoking transition(a, b) where 
> a==b.  If you'd prefer, i can allow that and return a NO_OP action, but all 
> that really does is move code around.  I don't have a strong opinion, neither 
> seems more attractive:
> 
> ```
> if (current != next) {
>   switch (transition(current, next)) {
> case STOP_WATCHING:
>   ...
>   }
> }
> ```
> 
> ```
> switch (transition(current, next)) {
>   case NO_OP:
> break;
>   case STOP_WATCHING:
> ...
> }
> ```

Just realized this may be a case of different definitions of _caller_.  My 
_caller_ is the next level up in the call stack, sounds yours is an HTTP API 
consumer.


- Bill


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


On Sept. 3, 2014, 4:02 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25300/
> ---
> 
> (Updated Sept. 3, 2014, 4:02 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This will be used by the job updater to gate insertion of JobUpdateEvents.
> 
> I chose not to use StateMachine here as it actually added complexity - in 
> this case we don't benefit from transition callbacks, and the result is based 
> only on the target state (rather than the transition).
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/25300/diff/
> 
> 
> Testing
> ---
> 
> `./gradlew build -Pq`
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 25300: Add a state machine to react to job update status changes.

[Bill Farner]

> On Sept. 3, 2014, 5:33 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java,
> >  lines 74-76
> > 
> >
> > This will also throw for in-place transitions (e.g. ROLL_FORWARD_PAUSED 
> > -> ROLL_FORWARD_PAUSED) making it harder to implement idempotent thrift 
> > APIs (i.e. pause/resume/abort). Would it make sense to alter 
> > ALLOWED_TRANSITIONS with same state no-op transitions or throw here only if 
> > from != to?
> 
> Bill Farner wrote:
> I didn't see much value in supporting no-op transitions.  The caller can 
> suppress that easily enough, so i chose to be more restrictive here.
> 
> Maxim Khutornenko wrote:
> I guess I don't see how the caller would be able to set apart illegal 
> state transition (valid error) from the same state one. Are you suggesting 
> querying the state before trying to call into the controller?

I'm suggesting that the caller avoid invoking transition(a, b) where a==b.  If 
you'd prefer, i can allow that and return a NO_OP action, but all that really 
does is move code around.  I don't have a strong opinion, neither seems more 
attractive:

```
if (current != next) {
  switch (transition(current, next)) {
case STOP_WATCHING:
  ...
  }
}
```

```
switch (transition(current, next)) {
  case NO_OP:
break;
  case STOP_WATCHING:
...
}
```


- Bill


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


On Sept. 3, 2014, 4:02 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25300/
> ---
> 
> (Updated Sept. 3, 2014, 4:02 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This will be used by the job updater to gate insertion of JobUpdateEvents.
> 
> I chose not to use StateMachine here as it actually added complexity - in 
> this case we don't benefit from transition callbacks, and the result is based 
> only on the target state (rather than the transition).
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/25300/diff/
> 
> 
> Testing
> ---
> 
> `./gradlew build -Pq`
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 25300: Add a state machine to react to job update status changes.

[Maxim Khutornenko]

> On Sept. 3, 2014, 5:33 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java,
> >  lines 74-76
> > 
> >
> > This will also throw for in-place transitions (e.g. ROLL_FORWARD_PAUSED 
> > -> ROLL_FORWARD_PAUSED) making it harder to implement idempotent thrift 
> > APIs (i.e. pause/resume/abort). Would it make sense to alter 
> > ALLOWED_TRANSITIONS with same state no-op transitions or throw here only if 
> > from != to?
> 
> Bill Farner wrote:
> I didn't see much value in supporting no-op transitions.  The caller can 
> suppress that easily enough, so i chose to be more restrictive here.

I guess I don't see how the caller would be able to set apart illegal state 
transition (valid error) from the same state one. Are you suggesting querying 
the state before trying to call into the controller?


- Maxim


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


On Sept. 3, 2014, 4:02 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25300/
> ---
> 
> (Updated Sept. 3, 2014, 4:02 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This will be used by the job updater to gate insertion of JobUpdateEvents.
> 
> I chose not to use StateMachine here as it actually added complexity - in 
> this case we don't benefit from transition callbacks, and the result is based 
> only on the target state (rather than the transition).
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/25300/diff/
> 
> 
> Testing
> ---
> 
> `./gradlew build -Pq`
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 25300: Add a state machine to react to job update status changes.

[Bill Farner]

> On Sept. 3, 2014, 5:33 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java,
> >  lines 74-76
> > 
> >
> > This will also throw for in-place transitions (e.g. ROLL_FORWARD_PAUSED 
> > -> ROLL_FORWARD_PAUSED) making it harder to implement idempotent thrift 
> > APIs (i.e. pause/resume/abort). Would it make sense to alter 
> > ALLOWED_TRANSITIONS with same state no-op transitions or throw here only if 
> > from != to?

I didn't see much value in supporting no-op transitions.  The caller can 
suppress that easily enough, so i chose to be more restrictive here.


- Bill


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


On Sept. 3, 2014, 4:02 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25300/
> ---
> 
> (Updated Sept. 3, 2014, 4:02 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This will be used by the job updater to gate insertion of JobUpdateEvents.
> 
> I chose not to use StateMachine here as it actually added complexity - in 
> this case we don't benefit from transition callbacks, and the result is based 
> only on the target state (rather than the transition).
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/25300/diff/
> 
> 
> Testing
> ---
> 
> `./gradlew build -Pq`
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 25300: Add a state machine to react to job update status changes.

[Maxim Khutornenko]

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



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java


This will also throw for in-place transitions (e.g. ROLL_FORWARD_PAUSED -> 
ROLL_FORWARD_PAUSED) making it harder to implement idempotent thrift APIs (i.e. 
pause/resume/abort). Would it make sense to alter ALLOWED_TRANSITIONS with same 
state no-op transitions or throw here only if from != to?


- Maxim Khutornenko


On Sept. 3, 2014, 4:02 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25300/
> ---
> 
> (Updated Sept. 3, 2014, 4:02 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This will be used by the job updater to gate insertion of JobUpdateEvents.
> 
> I chose not to use StateMachine here as it actually added complexity - in 
> this case we don't benefit from transition callbacks, and the result is based 
> only on the target state (rather than the transition).
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/25300/diff/
> 
> 
> Testing
> ---
> 
> `./gradlew build -Pq`
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Re: Review Request 25300: Add a state machine to react to job update status changes.

[Joshua Cohen]

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

Ship it!


Ship It!

- Joshua Cohen


On Sept. 3, 2014, 4:02 p.m., Bill Farner wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25300/
> ---
> 
> (Updated Sept. 3, 2014, 4:02 p.m.)
> 
> 
> Review request for Aurora, Joshua Cohen and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-613
> https://issues.apache.org/jira/browse/AURORA-613
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This will be used by the job updater to gate insertion of JobUpdateEvents.
> 
> I chose not to use StateMachine here as it actually added complexity - in 
> this case we don't benefit from transition callbacks, and the result is based 
> only on the target state (rather than the transition).
> 
> 
> Diffs
> -
> 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 
> PRE-CREATION 
>   
> src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/25300/diff/
> 
> 
> Testing
> ---
> 
> `./gradlew build -Pq`
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Review Request 25300: Add a state machine to react to job update status changes.

[Bill Farner]

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

Review request for Aurora, Joshua Cohen and Maxim Khutornenko.


Bugs: AURORA-613
https://issues.apache.org/jira/browse/AURORA-613


Repository: aurora


Description
---

This will be used by the job updater to gate insertion of JobUpdateEvents.

I chose not to use StateMachine here as it actually added complexity - in this 
case we don't benefit from transition callbacks, and the result is based only 
on the target state (rather than the transition).


Diffs
-

  src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java 
PRE-CREATION 
  
src/test/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachineTest.java
 PRE-CREATION 

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


Testing
---

`./gradlew build -Pq`


Thanks,

Bill Farner