aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Maxim Khutornenko" <ma...@apache.org>
Subject Re: Review Request 25300: Add a state machine to react to job update status changes.
Date Wed, 03 Sep 2014 19:24:31 GMT


> On Sept. 3, 2014, 5:33 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateStateMachine.java, lines
74-76
> > <https://reviews.apache.org/r/25300/diff/1/?file=675222#file675222line74>
> >
> >     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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message