> 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. > > Maxim Khutornenko wrote: > 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. > > Bill Farner wrote: > Is this true? If other callers can manage to effect a transition attempt > to DELETED, why would it be harder for tests?
I am not saying it's impossible, it just does not feel right here. If we hide the enum the TaskStateMachineTest would not be able to define transitions similar to this: .put(new TestCase(true, KILLED, DELETED), DELETE_TASK) I am not even sure how to test transitions like this one: .put(new TestCase(false, DELETED, ASSIGNED), ILLEGAL_KILL) .put(new TestCase(false, DELETED, STARTING), ILLEGAL_KILL) .put(new TestCase(false, DELETED, RUNNING), ILLEGAL_KILL) The only way would be to force the task state into DELETED by issuing an updateState(), assume the state is DELETED and then attempt to move into one of the non-terminal states expecting the ILLEGAL_KILL. This approach will lose readability and coverage confidence. > 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()? > > Maxim Khutornenko wrote: > 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. > > Bill Farner wrote: > > from the state machine perspective > > This is true, but it seems weird, and slightly restricting, to effect > deletion by asking to transition to lack of a state. This works now, but not > if we ever want to do other things with TaskStateMachine that are not > represented by ScheduleStatus values. Your call here. I'd rather deal with it when/if the time comes to add another absent state. Until then leak avoidance feels a higher priority to me. > 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. > > Maxim Khutornenko wrote: > 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. > > Bill Farner wrote: > How about a separate test case? Seems like we lost some coverage by > flipping the states tested against here. I'd argue that the coverage has actually improved here :) All HistoryPruner cares is that the task state is terminal. There is no difference between LOST, SANDBOX_DELETED or any other state as far as block/arc coverage goes. We do have full representation of the terminals now after I sprinkled a few SANDBOX_DELETED states in places where states were repeated. - Maxim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19648/#review38779 ----------------------------------------------------------- On March 27, 2014, 7:59 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, 7:59 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 > >
