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


Biggest dislike is the new Optional argument.  I'd rather retain the explicit 
API for deletion, probably all the way into TaskStateMachine.


src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java
<https://reviews.apache.org/r/19648/#comment70897>

    This API doesn't provide very good ergonomics.  (Frankly, it was already 
fairly poor with the existing Optional, my fault there.)  From the signature, 
it's not clear what, if anything, should happen when passing two absent 
ScheduleStatus arguments.  And i would classify the behavior as surprising.
    
    What made you decide to remove deleteTasks?



src/main/java/org/apache/aurora/scheduler/base/Tasks.java
<https://reviews.apache.org/r/19648/#comment70896>

    Why is SANDBOX_DELETED not considered a terminal state?



src/main/thrift/org/apache/aurora/gen/api.thrift
<https://reviews.apache.org/r/19648/#comment70903>

    The statement about state reconciliation should probably go, i don't think 
it applies any more.



src/test/java/org/apache/aurora/scheduler/MesosSchedulerImplTest.java
<https://reviews.apache.org/r/19648/#comment70905>

    testSandboxesDeleted?



src/test/java/org/apache/aurora/scheduler/MesosSchedulerImplTest.java
<https://reviews.apache.org/r/19648/#comment70904>

    s/final // for all 3


- Bill Farner


On March 25, 2014, 11:44 p.m., Maxim Khutornenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19648/
> -----------------------------------------------------------
> 
> (Updated March 25, 2014, 11:44 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/UserTaskLauncher.java 
> fd2644172e3814e8cf5f976753b07f6196368d71 
>   src/main/java/org/apache/aurora/scheduler/async/HistoryPruner.java 
> 59f615c89ba1fad1656934da7dca6bd4ed741739 
>   src/main/java/org/apache/aurora/scheduler/async/Preemptor.java 
> c3bf8c945b1f0b0923941660dd3eb57a5b614541 
>   src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 
> 263235ce7c279f98920d7681f6162458be40593c 
>   src/main/java/org/apache/aurora/scheduler/async/TaskThrottler.java 
> 2334e7ff30843be4cbdd9086cbdafa61bb15ac94 
>   src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 
> de79912bbd4825bba50e2d7be8eee5fd613b76d4 
>   src/main/java/org/apache/aurora/scheduler/base/Jobs.java 
> 4ac218b8f25be5e9e6b6243558af4258efe6e61a 
>   src/main/java/org/apache/aurora/scheduler/base/Query.java 
> 1e586c5ecc52ea32e50468942fd00a2d85463281 
>   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/MaintenanceController.java 
> 155fdd1e31bf52b61aab0ebbffb5264af644a38c 
>   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/UserTaskLauncherTest.java 
> 44cb20ebda1fc368a2570229e8294a266e858f57 
>   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/async/PreemptorImplTest.java 
> 24a35bf201077572b7973c4e46a46204ee3cd9a0 
>   src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 
> ce03abc5248dbc95306c759a04828830e3057b81 
>   src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java 
> a539b5931ed8b3a4a3e252e92ca1644b0c51a7fc 
>   src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java 
> bd1f599f889f1390731f520dfed9a3087bcaf00b 
>   src/test/java/org/apache/aurora/scheduler/base/JobsTest.java 
> effe14bf36d967b64a30af77378f106b862944cd 
>   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/MaintenanceControllerImplTest.java
>  faa38855438d2b147d32a158b3ebcd58089a7fc5 
>   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