> On March 27, 2014, 7:09 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/base/Tasks.java, line 232
> > <https://reviews.apache.org/r/19648/diff/4/?file=538559#file538559line232>
> >
> >     Both callers immediately call get() on the result.  Seems sane with the 
> > signature for this to return non-optional and throw.
> >     
> >     That should also reduce the implementation to a one-liner.

Planning for future callers that might never come. Changed.


> On March 27, 2014, 7:09 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/StateManager.java, line 83
> > <https://reviews.apache.org/r/19648/diff/4/?file=538563#file538563line83>
> >
> >     "Attempts to delete tasks from the task store."

Changed.


> On March 27, 2014, 7:09 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/StateManager.java, line 84
> > <https://reviews.apache.org/r/19648/diff/4/?file=538563#file538563line84>
> >
> >     DELETED is an implementation detail of TaskStateMachine, so it should 
> > not be referenced here.
> >     
> >     How about: "If the task is not currently in a state that is considered 
> > safe for deletion, side-effect actions will be performed to reconcile the 
> > state conflict."

Done.


> On March 27, 2014, 7:09 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java, line 
> > 107
> > <https://reviews.apache.org/r/19648/diff/4/?file=538565#file538565line107>
> >
> >     Would it take much to make this private?  It would be nice if nothing 
> > (including the unit test) knew about this detail.

That would make tests much harder, especially testing legal transitions to 
DELETED state. Gain on the source side would be a loss on the test side. Given 
the complexity of the state machine I would rather go with better tests here. 


> On March 27, 2014, 7:09 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java, line 
> > 534
> > <https://reviews.apache.org/r/19648/diff/4/?file=538565#file538565line534>
> >
> >     How would you feel about an explicit delete function instead of 
> > signaling that with Optional.absent()?

It does not seem like a sound approach here. The DELETED state is not much 
different than any other from the state machine perspective. Splitting the 
delete out of it would mean leaking the state semantic outside of the state 
machine, i.e. the StateManagerImpl would have to special case transition to 
DELETED state.


> On March 27, 2014, 7:09 p.m., Bill Farner wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 250
> > <https://reviews.apache.org/r/19648/diff/4/?file=538568#file538568line250>
> >
> >     TODO(somebody, maybe you): Remove INIT state.

Ah, forgot this one. Added.


> On March 27, 2014, 7:09 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java, 
> > line 107
> > <https://reviews.apache.org/r/19648/diff/4/?file=538571#file538571line107>
> >
> >     Why were some of these states changed?  AFAICT they should not have 
> > been affected.

Correct. All I am doing here is improving a variety of terminal states to make 
sure SANDBOX_DELETED is picked up for deletion by the HistoryPruner query 
together with other terminals.


> On March 27, 2014, 7:09 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java, 
> > line 293
> > <https://reviews.apache.org/r/19648/diff/4/?file=538571#file538571line293>
> >
> >     The reasoning for Void->Boolean is not clear to me, what's going on 
> > here?

Leftover from the previous stateChange() approach. Reverted.


> On March 27, 2014, 7:09 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java, 
> > line 347
> > <https://reviews.apache.org/r/19648/diff/4/?file=538571#file538571line347>
> >
> >     Why the Set->Array->Set dance?

Attempt to reuse the existing method. Refactored in favor of direct call.


> On March 27, 2014, 7:09 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/base/TaskTestUtil.java, line 63
> > <https://reviews.apache.org/r/19648/diff/4/?file=538573#file538573line63>
> >
> >     The implicit null was bad, but the explicit one is just as bad.  What's 
> > driving the change here?  Can you use a non-null status?

Don't have a preference here. Having null instead of status might not be what 
all tests expect though. Changed anyway.


> On March 27, 2014, 7:09 p.m., Bill Farner wrote:
> > src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java,
> >  line 1017
> > <https://reviews.apache.org/r/19648/diff/4/?file=538575#file538575line1017>
> >
> >     This causes us to lose test coverage on carrying forward ancestor IDs.  
> > Can you make sure to patch that up somewhere?  StateManagerImplTest seems 
> > like the most appropriate place.

Sure, done.


- Maxim


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


On March 27, 2014, 6:09 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19648/
> -----------------------------------------------------------
> 
> (Updated March 27, 2014, 6:09 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Suman Karumuri, and Bill Farner.
> 
> 
> Bugs: AURORA-261
>     https://issues.apache.org/jira/browse/AURORA-261
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Part 2:
> - Renaming UNKNOWN to SANDBOX_DELETED;
> - Persisting SANDBOX_DELETED on legal transitions to ensure history is 
> retained;
> - Adding a new DELETED volatile state to guide task deletion on absent 
> ScheduleStatus value;
> - Dropping "delete" methods from SchedulerCore and StateManager interfaces to 
> ensure all task deletion goes through DELETED state change;
> - UI changes to report terminal state in case of GC deletion (screenshots 
> attached).
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/MesosSchedulerImpl.java 
> 772511f74fc12cf320f9eb4dcc857c58a3546aa1 
>   src/main/java/org/apache/aurora/scheduler/base/Jobs.java 
> 4ac218b8f25be5e9e6b6243558af4258efe6e61a 
>   src/main/java/org/apache/aurora/scheduler/base/Tasks.java 
> 003d475a1bd4ecc099d9a641fd239a8189f71cdb 
>   src/main/java/org/apache/aurora/scheduler/http/SchedulerzJob.java 
> 2ccc6f367b9715a0abb3e0673069289ae4860087 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java 
> 7a13a8b9e8c9b7a767ec4bc4201e97de17809b82 
>   src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java 
> 3ca4ee529aaf3491118da216116b19d7c6a49d09 
>   src/main/java/org/apache/aurora/scheduler/state/StateManager.java 
> 4249707b283da078321faaaed006de54519238bc 
>   src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 
> 0161fac838ff5166161acd2960058de6856b0e9c 
>   src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java 
> d2becea60e5d7bb59a2e5adb66e10cd50f6b56f3 
>   src/main/python/apache/aurora/executor/gc_executor.py 
> 4a866b2d6041a592692812eb3472db744d21e194 
>   src/main/resources/org/apache/aurora/scheduler/http/schedulerzjob.st 
> 28b56671b2e825912a6427e609c2bbe1e7758e26 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> c0618e4edebd6f282698abfd9bdc3c36fff16920 
>   src/test/java/org/apache/aurora/scheduler/MesosSchedulerImplTest.java 
> 534b2b62cc2ae0ae9baab0247c9679350f03042b 
>   src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java 
> a4e9464f7d5d3f5a640b62557c3e29f2f1566985 
>   src/test/java/org/apache/aurora/scheduler/async/HistoryPrunerTest.java 
> 378ece964081a7cb475dc6edd9b19ed506f03ec8 
>   src/test/java/org/apache/aurora/scheduler/base/JobsTest.java 
> effe14bf36d967b64a30af77378f106b862944cd 
>   src/test/java/org/apache/aurora/scheduler/base/TaskTestUtil.java 
> 2006ddb7f83a8ad2a1ce6ff3a96faf3c0bd8d8e9 
>   src/test/java/org/apache/aurora/scheduler/base/TasksTest.java 
> 102fe04f8d3a7142a0cd58251e16d31e8d4a433d 
>   
> src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java
>  033ebd29a4e95914347c1f5f8ade73115585a26f 
>   src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java 
> 24949c043b16a35a7960ed0a40b79f0981179eae 
>   src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java 
> e77063a9c8e40e015ec264b151a7ed76f1c7f00f 
>   src/test/resources/org/apache/aurora/gen/api.thrift.md5 
> 05c6e8a1e2407f2822b3844c555d8995f1cd1d49 
> 
> Diff: https://reviews.apache.org/r/19648/diff/
> 
> 
> Testing
> -------
> 
> ./build-support/jenkins/build.sh 
> 
> 
> File Attachments
> ----------------
> 
> Finished
>   
> https://reviews.apache.org/media/uploaded/files/2014/03/25/c5dd85ec-14df-4744-a2f9-d3af089e6286__CompletedFinished.png
> Deleted
>   
> https://reviews.apache.org/media/uploaded/files/2014/03/25/5bd3f5ec-89f8-4c91-a034-499a8db4caf9__CompletedDeleted.png
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Reply via email to