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 19648: Preserving sandbox deleted task history.
Date Thu, 27 Mar 2014 19:58:56 GMT


> 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
> 
>


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