aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Farner" <wfar...@apache.org>
Subject Re: Review Request 24465: Add a one-way job update controller.
Date Mon, 11 Aug 2014 20:15:46 GMT


> On Aug. 8, 2014, 4:17 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java, line 41
> > <https://reviews.apache.org/r/24465/diff/2/?file=655800#file655800line41>
> >
> >     Missing @param <K> comment.

Fixed.


> On Aug. 8, 2014, 4:17 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java, line 65
> > <https://reviews.apache.org/r/24465/diff/2/?file=655800#file655800line65>
> >
> >     How would instance events propagate from here? From what I can see the caller
would not have the instance state exposure, so I am assuming it's going to be another "instanceEventSink"
constructor argument?

Presumably events will directly correlation with actions taken on tasks, so that suggests
they will be translated from InstanceActions.


> On Aug. 8, 2014, 4:17 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java, lines 130-132
> > <https://reviews.apache.org/r/24465/diff/2/?file=655800#file655800line130>
> >
> >     This method is a bit hard to follow given the presence of both update and instance
state machine transitions. How about encapsulating instance transitions in helper methods?
Would make the algorithm here more readable.

Done, pulled the job-level transition out.


> On Aug. 8, 2014, 4:17 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java, lines 170-171
> > <https://reviews.apache.org/r/24465/diff/2/?file=655800#file655800line170>
> >
> >     This would make it impossible to do a failure-agnostic rollback we currently
have in client. Are you suggesting the rollback would be a best effort aborting in case of
maxFailedInstances reached?

There are a few areas that need to be revisited for rollback, which should not be major. 
Added a TODO.


> On Aug. 8, 2014, 4:17 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java, lines 237-245
> > <https://reviews.apache.org/r/24465/diff/2/?file=655800#file655800line237>
> >
> >     I thought the thrift JobUpdateAction is what we would use here, no?

Actions will be a subset of this enum.


- Bill


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


On Aug. 7, 2014, 11:11 p.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24465/
> -----------------------------------------------------------
> 
> (Updated Aug. 7, 2014, 11:11 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-613
>     https://issues.apache.org/jira/browse/AURORA-613
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> There are 3 levels to performing an update:
> 
> 1. Move the job from state A to state B, roll back on failure
> 2. Take a job from state A to state B
> 3. Take an instance from state A to state B
> 
> This implements level 2.  I made the OneWayJobUpdater generic, which actually simplified
both implementation and testing, since it is only responsible for relaying that state down
to level 3.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/updater/InstanceStateProvider.java PRE-CREATION

>   src/main/java/org/apache/aurora/scheduler/updater/InstanceUpdater.java 7476d82e9691449fa968c5fc4c5af76837a5c9cf

>   src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java PRE-CREATION

>   src/main/java/org/apache/aurora/scheduler/updater/StateEvaluator.java PRE-CREATION

>   src/test/java/org/apache/aurora/scheduler/updater/InstanceUpdaterTest.java dda1b73ead847e5ee9c5c7bc8be3cd8a7f59ac80

>   src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterTest.java PRE-CREATION

> 
> Diff: https://reviews.apache.org/r/24465/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew build -Pq
> 
> OneWayJobUpdater has 100% instruction and branch coverage.
> 
> 
> Thanks,
> 
> Bill Farner
> 
>


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