aurora-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ma...@apache.org
Subject aurora git commit: Enhancing the StateManager.changeState result.
Date Wed, 13 May 2015 18:09:59 GMT
Repository: aurora
Updated Branches:
  refs/heads/master 7eba711c3 -> 2dc1d59f1


Enhancing the StateManager.changeState result.

Reviewed at https://reviews.apache.org/r/34148/


Project: http://git-wip-us.apache.org/repos/asf/aurora/repo
Commit: http://git-wip-us.apache.org/repos/asf/aurora/commit/2dc1d59f
Tree: http://git-wip-us.apache.org/repos/asf/aurora/tree/2dc1d59f
Diff: http://git-wip-us.apache.org/repos/asf/aurora/diff/2dc1d59f

Branch: refs/heads/master
Commit: 2dc1d59f1e772e220b3bfb26480c3b90c688800f
Parents: 7eba711
Author: Maxim Khutornenko <maxim@apache.org>
Authored: Wed May 13 11:08:42 2015 -0700
Committer: Maxim Khutornenko <maxim@apache.org>
Committed: Wed May 13 11:08:42 2015 -0700

----------------------------------------------------------------------
 .../aurora/scheduler/async/TaskTimeout.java     |  7 +--
 .../scheduler/state/StateChangeResult.java      | 45 ++++++++++++++++++++
 .../aurora/scheduler/state/StateManager.java    |  6 +--
 .../scheduler/state/StateManagerImpl.java       | 16 ++++---
 .../scheduler/state/TaskStateMachine.java       | 13 +++++-
 .../scheduler/state/TransitionResult.java       | 27 ++++++++----
 .../thrift/SchedulerThriftInterface.java        |  3 +-
 .../aurora/scheduler/UserTaskLauncherTest.java  |  5 ++-
 .../scheduler/async/TaskSchedulerTest.java      |  3 +-
 .../scheduler/async/TaskThrottlerTest.java      |  3 +-
 .../aurora/scheduler/async/TaskTimeoutTest.java |  7 +--
 .../async/preemptor/PreemptorImplTest.java      |  3 +-
 .../cron/quartz/AuroraCronJobTest.java          |  3 +-
 .../state/MaintenanceControllerImplTest.java    |  2 +-
 .../scheduler/state/StateManagerImplTest.java   | 32 +++++++-------
 .../scheduler/state/TaskStateMachineTest.java   | 41 +++++++++++-------
 .../thrift/SchedulerThriftInterfaceTest.java    |  7 +--
 .../aurora/scheduler/updater/JobUpdaterIT.java  |  4 +-
 18 files changed, 156 insertions(+), 71 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/aurora/blob/2dc1d59f/src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java
----------------------------------------------------------------------
diff --git a/src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java b/src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java
index 90e6149..e250f33 100644
--- a/src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java
+++ b/src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java
@@ -32,6 +32,7 @@ import com.twitter.common.stats.StatsProvider;
 import org.apache.aurora.gen.ScheduleStatus;
 import org.apache.aurora.scheduler.events.PubsubEvent.EventSubscriber;
 import org.apache.aurora.scheduler.events.PubsubEvent.TaskStateChange;
+import org.apache.aurora.scheduler.state.StateChangeResult;
 import org.apache.aurora.scheduler.state.StateManager;
 import org.apache.aurora.scheduler.storage.Storage;
 
@@ -116,9 +117,9 @@ class TaskTimeout extends AbstractIdleService implements EventSubscriber
{
         // canceled, but in the event of a state transition race, including transientState
         // prevents an unintended task timeout.
         // Note: This requires LOST transitions trigger Driver.killTask.
-        boolean movedToLost = storage.write(new MutateWork.Quiet<Boolean>() {
+        StateChangeResult result = storage.write(new MutateWork.Quiet<StateChangeResult>()
{
           @Override
-          public Boolean apply(Storage.MutableStoreProvider storeProvider) {
+          public StateChangeResult apply(Storage.MutableStoreProvider storeProvider) {
             return stateManager.changeState(
                 storeProvider,
                 taskId,
@@ -128,7 +129,7 @@ class TaskTimeout extends AbstractIdleService implements EventSubscriber
{
           }
         });
 
-        if (movedToLost) {
+        if (result == StateChangeResult.SUCCESS) {
           LOG.info("Timeout reached for task " + taskId + ":" + taskId);
           timedOutTasks.incrementAndGet();
         }

http://git-wip-us.apache.org/repos/asf/aurora/blob/2dc1d59f/src/main/java/org/apache/aurora/scheduler/state/StateChangeResult.java
----------------------------------------------------------------------
diff --git a/src/main/java/org/apache/aurora/scheduler/state/StateChangeResult.java b/src/main/java/org/apache/aurora/scheduler/state/StateChangeResult.java
new file mode 100644
index 0000000..d64a776
--- /dev/null
+++ b/src/main/java/org/apache/aurora/scheduler/state/StateChangeResult.java
@@ -0,0 +1,45 @@
+/**
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.aurora.scheduler.state;
+
+/**
+ * The result of a task state change request.
+ */
+public enum StateChangeResult {
+  /**
+   * Task is already in target state.
+   */
+  NOOP,
+
+  /**
+   * Task state has been changed successfully.
+   */
+  SUCCESS,
+
+  /**
+   * Change is illegal given the current task state.
+   */
+  ILLEGAL,
+
+  /**
+   * Same as {@link #ILLEGAL} but processing state change request generated some task side
effects
+   * (e.g.: a driver.kill() request has been placed).
+   */
+  ILLEGAL_WITH_SIDE_EFFECTS,
+
+  /**
+   * Change is not allowed due to the task not in CAS state.
+   */
+  INVALID_CAS_STATE
+}

http://git-wip-us.apache.org/repos/asf/aurora/blob/2dc1d59f/src/main/java/org/apache/aurora/scheduler/state/StateManager.java
----------------------------------------------------------------------
diff --git a/src/main/java/org/apache/aurora/scheduler/state/StateManager.java b/src/main/java/org/apache/aurora/scheduler/state/StateManager.java
index 71bfefb..5d34fe3 100644
--- a/src/main/java/org/apache/aurora/scheduler/state/StateManager.java
+++ b/src/main/java/org/apache/aurora/scheduler/state/StateManager.java
@@ -44,12 +44,10 @@ public interface StateManager {
    *                 decision to defer the action was mde.
    * @param newState State to move the task to.
    * @param auditMessage Message to include with the transition.
-   * @return {@code true} if the transition was performed and the task was moved to
-   *         {@code newState}, {@code false} if the transition was not allowed, or the task
was not
-   *         in {@code casState}.
+   * @return {@link StateChangeResult}.
    *
    */
-  boolean changeState(
+  StateChangeResult changeState(
       MutableStoreProvider storeProvider,
       String taskId,
       Optional<ScheduleStatus> casState,

http://git-wip-us.apache.org/repos/asf/aurora/blob/2dc1d59f/src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java
----------------------------------------------------------------------
diff --git a/src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java b/src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java
index d87bb38..1b8733b 100644
--- a/src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java
+++ b/src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java
@@ -68,6 +68,8 @@ import static org.apache.aurora.gen.ScheduleStatus.ASSIGNED;
 import static org.apache.aurora.gen.ScheduleStatus.INIT;
 import static org.apache.aurora.gen.ScheduleStatus.PENDING;
 import static org.apache.aurora.gen.ScheduleStatus.THROTTLED;
+import static org.apache.aurora.scheduler.state.StateChangeResult.INVALID_CAS_STATE;
+import static org.apache.aurora.scheduler.state.StateChangeResult.SUCCESS;
 
 /**
  * Manager of all persistence-related operations for the scheduler.  Acts as a controller
for
@@ -149,7 +151,7 @@ public class StateManagerImpl implements StateManager {
   }
 
   @Override
-  public boolean changeState(
+  public StateChangeResult changeState(
       MutableStoreProvider storeProvider,
       String taskId,
       Optional<ScheduleStatus> casState,
@@ -192,7 +194,7 @@ public class StateManagerImpl implements StateManager {
           }
         });
 
-    boolean success = updateTaskAndExternalState(
+    StateChangeResult changeResult = updateTaskAndExternalState(
         storeProvider.getUnsafeTaskStore(),
         Optional.<ScheduleStatus>absent(),
         taskId,
@@ -200,7 +202,7 @@ public class StateManagerImpl implements StateManager {
         Optional.<String>absent());
 
     Preconditions.checkState(
-        success,
+        changeResult == SUCCESS,
         "Attempt to assign task " + taskId + " to " + slaveHost + " failed");
 
     return Iterables.getOnlyElement(
@@ -223,7 +225,7 @@ public class StateManagerImpl implements StateManager {
         }
       });
 
-  private boolean updateTaskAndExternalState(
+  private StateChangeResult updateTaskAndExternalState(
       TaskStore.Mutable taskStore,
       Optional<ScheduleStatus> casState,
       String taskId,
@@ -238,7 +240,7 @@ public class StateManagerImpl implements StateManager {
     if (casState.isPresent()
         && (!task.isPresent() || casState.get() != task.get().getStatus())) {
 
-      return false;
+      return INVALID_CAS_STATE;
     }
 
     return updateTaskAndExternalState(
@@ -277,7 +279,7 @@ public class StateManagerImpl implements StateManager {
   private static final Ordering<SideEffect> ACTION_ORDER =
       Ordering.explicit(ACTIONS_IN_ORDER).onResultOf(GET_ACTION);
 
-  private boolean updateTaskAndExternalState(
+  private StateChangeResult updateTaskAndExternalState(
       TaskStore.Mutable taskStore,
       String taskId,
       // Note: This argument is deliberately non-final, and should not be made final.
@@ -399,7 +401,7 @@ public class StateManagerImpl implements StateManager {
       eventSink.post(event);
     }
 
-    return result.isSuccess();
+    return result.getResult();
   }
 
   @Override

http://git-wip-us.apache.org/repos/asf/aurora/blob/2dc1d59f/src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java
----------------------------------------------------------------------
diff --git a/src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java b/src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java
index 4a7ca62..48d0ff6 100644
--- a/src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java
+++ b/src/main/java/org/apache/aurora/scheduler/state/TaskStateMachine.java
@@ -48,6 +48,10 @@ import static org.apache.aurora.scheduler.state.SideEffect.Action.INCREMENT_FAIL
 import static org.apache.aurora.scheduler.state.SideEffect.Action.KILL;
 import static org.apache.aurora.scheduler.state.SideEffect.Action.RESCHEDULE;
 import static org.apache.aurora.scheduler.state.SideEffect.Action.SAVE_STATE;
+import static org.apache.aurora.scheduler.state.StateChangeResult.ILLEGAL;
+import static org.apache.aurora.scheduler.state.StateChangeResult.ILLEGAL_WITH_SIDE_EFFECTS;
+import static org.apache.aurora.scheduler.state.StateChangeResult.NOOP;
+import static org.apache.aurora.scheduler.state.StateChangeResult.SUCCESS;
 import static org.apache.aurora.scheduler.state.TaskStateMachine.TaskState.ASSIGNED;
 import static org.apache.aurora.scheduler.state.TaskStateMachine.TaskState.DELETED;
 import static org.apache.aurora.scheduler.state.TaskStateMachine.TaskState.DRAINING;
@@ -523,13 +527,18 @@ class TaskStateMachine {
      */
     TaskState taskState = status.transform(STATUS_TO_TASK_STATE).or(TaskState.DELETED);
     if (stateMachine.getState() == taskState) {
-      return new TransitionResult(false, ImmutableSet.<SideEffect>of());
+      return new TransitionResult(NOOP, ImmutableSet.<SideEffect>of());
     }
 
     boolean success = stateMachine.transition(taskState);
     ImmutableSet<SideEffect> transitionEffects = ImmutableSet.copyOf(sideEffects);
     sideEffects.clear();
-    return new TransitionResult(success, transitionEffects);
+    if (success) {
+      return new TransitionResult(SUCCESS, transitionEffects);
+    }
+    return new TransitionResult(
+          transitionEffects.isEmpty() ? ILLEGAL : ILLEGAL_WITH_SIDE_EFFECTS,
+          transitionEffects);
   }
 
   /**

http://git-wip-us.apache.org/repos/asf/aurora/blob/2dc1d59f/src/main/java/org/apache/aurora/scheduler/state/TransitionResult.java
----------------------------------------------------------------------
diff --git a/src/main/java/org/apache/aurora/scheduler/state/TransitionResult.java b/src/main/java/org/apache/aurora/scheduler/state/TransitionResult.java
index 874c554..6928c66 100644
--- a/src/main/java/org/apache/aurora/scheduler/state/TransitionResult.java
+++ b/src/main/java/org/apache/aurora/scheduler/state/TransitionResult.java
@@ -15,30 +15,39 @@ package org.apache.aurora.scheduler.state;
 
 import java.util.Objects;
 
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableSet;
 
+import static org.apache.aurora.scheduler.state.StateChangeResult.ILLEGAL_WITH_SIDE_EFFECTS;
+import static org.apache.aurora.scheduler.state.StateChangeResult.SUCCESS;
+
 /**
  * The actions that should be performed in response to a state transition attempt.
  *
  * {@see TaskStateMachine}
  */
 public class TransitionResult {
-  private final boolean success;
+  private final StateChangeResult result;
   private final ImmutableSet<SideEffect> sideEffects;
 
   /**
    * Creates a transition result with the given side effects.
    *
-   * @param success Whether the transition attempt relevant to this result was successful.
+   * @param result Transition attempt result.
    * @param sideEffects Actions that must be performed in response to the state transition.
    */
-  public TransitionResult(boolean success, ImmutableSet<SideEffect> sideEffects) {
-    this.success = success;
+  public TransitionResult(StateChangeResult result, ImmutableSet<SideEffect> sideEffects)
{
+    this.result = result;
     this.sideEffects = Objects.requireNonNull(sideEffects);
+    if (!this.sideEffects.isEmpty()) {
+      Preconditions.checkArgument(
+          result == SUCCESS || result == ILLEGAL_WITH_SIDE_EFFECTS,
+          "Invalid transition result for a non-empty set of side effects");
+    }
   }
 
-  public boolean isSuccess() {
-    return success;
+  public StateChangeResult getResult() {
+    return result;
   }
 
   public ImmutableSet<SideEffect> getSideEffects() {
@@ -52,19 +61,19 @@ public class TransitionResult {
     }
 
     TransitionResult other = (TransitionResult) o;
-    return Objects.equals(success, other.success)
+    return Objects.equals(result, other.result)
         && Objects.equals(sideEffects, other.sideEffects);
   }
 
   @Override
   public int hashCode() {
-    return Objects.hash(success, sideEffects);
+    return Objects.hash(result, sideEffects);
   }
 
   @Override
   public String toString() {
     return com.google.common.base.Objects.toStringHelper(this)
-        .add("success", success)
+        .add("result", result)
         .add("sideEffects", sideEffects)
         .toString();
   }

http://git-wip-us.apache.org/repos/asf/aurora/blob/2dc1d59f/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
----------------------------------------------------------------------
diff --git a/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
b/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
index 160db12..1ca3a9b 100644
--- a/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
+++ b/src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
@@ -98,6 +98,7 @@ import org.apache.aurora.scheduler.quota.QuotaManager.QuotaException;
 import org.apache.aurora.scheduler.state.LockManager;
 import org.apache.aurora.scheduler.state.LockManager.LockException;
 import org.apache.aurora.scheduler.state.MaintenanceController;
+import org.apache.aurora.scheduler.state.StateChangeResult;
 import org.apache.aurora.scheduler.state.StateManager;
 import org.apache.aurora.scheduler.state.UUIDGenerator;
 import org.apache.aurora.scheduler.storage.CronJobStore;
@@ -570,7 +571,7 @@ class SchedulerThriftInterface implements AnnotatedAuroraAdmin {
 
         boolean tasksKilled = false;
         for (String taskId : Tasks.ids(tasks)) {
-          tasksKilled |= stateManager.changeState(
+          tasksKilled |= StateChangeResult.SUCCESS == stateManager.changeState(
               storeProvider,
               taskId,
               Optional.<ScheduleStatus>absent(),

http://git-wip-us.apache.org/repos/asf/aurora/blob/2dc1d59f/src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java
----------------------------------------------------------------------
diff --git a/src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java b/src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java
index 3243232..8da488d 100644
--- a/src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java
+++ b/src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java
@@ -21,6 +21,7 @@ import org.apache.aurora.gen.HostAttributes;
 import org.apache.aurora.gen.ScheduleStatus;
 import org.apache.aurora.scheduler.async.OfferManager;
 import org.apache.aurora.scheduler.mesos.Offers;
+import org.apache.aurora.scheduler.state.StateChangeResult;
 import org.apache.aurora.scheduler.state.StateManager;
 import org.apache.aurora.scheduler.storage.Storage.StorageException;
 import org.apache.aurora.scheduler.storage.entities.IHostAttributes;
@@ -78,7 +79,7 @@ public class UserTaskLauncherTest extends EasyMockTest {
         Optional.<ScheduleStatus>absent(),
         RUNNING,
         Optional.of("fake message")))
-        .andReturn(true);
+        .andReturn(StateChangeResult.SUCCESS);
 
     control.replay();
 
@@ -127,7 +128,7 @@ public class UserTaskLauncherTest extends EasyMockTest {
         Optional.<ScheduleStatus>absent(),
         FAILED,
         Optional.of(UserTaskLauncher.MEMORY_LIMIT_DISPLAY)))
-        .andReturn(false);
+        .andReturn(StateChangeResult.ILLEGAL);
 
     control.replay();
 

http://git-wip-us.apache.org/repos/asf/aurora/blob/2dc1d59f/src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java
----------------------------------------------------------------------
diff --git a/src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java b/src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java
index f17c434..f348541 100644
--- a/src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java
+++ b/src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java
@@ -54,6 +54,7 @@ import org.apache.aurora.scheduler.filter.AttributeAggregate;
 import org.apache.aurora.scheduler.filter.SchedulingFilter.ResourceRequest;
 import org.apache.aurora.scheduler.mesos.Driver;
 import org.apache.aurora.scheduler.state.MaintenanceController;
+import org.apache.aurora.scheduler.state.StateChangeResult;
 import org.apache.aurora.scheduler.state.StateManager;
 import org.apache.aurora.scheduler.state.TaskAssigner;
 import org.apache.aurora.scheduler.state.TaskAssigner.Assignment;
@@ -358,7 +359,7 @@ public class TaskSchedulerTest extends EasyMockTest {
         eq(Optional.of(PENDING)),
         eq(LOST),
         eq(TaskSchedulerImpl.LAUNCH_FAILED_MSG)))
-        .andReturn(true);
+        .andReturn(StateChangeResult.SUCCESS);
 
     replayAndCreateScheduler();
 

http://git-wip-us.apache.org/repos/asf/aurora/blob/2dc1d59f/src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java
----------------------------------------------------------------------
diff --git a/src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java b/src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java
index a637101..5772e15 100644
--- a/src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java
+++ b/src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java
@@ -29,6 +29,7 @@ import org.apache.aurora.gen.ScheduledTask;
 import org.apache.aurora.gen.TaskEvent;
 import org.apache.aurora.scheduler.base.Tasks;
 import org.apache.aurora.scheduler.events.PubsubEvent.TaskStateChange;
+import org.apache.aurora.scheduler.state.StateChangeResult;
 import org.apache.aurora.scheduler.state.StateManager;
 import org.apache.aurora.scheduler.storage.entities.IScheduledTask;
 import org.apache.aurora.scheduler.storage.testing.StorageTestUtil;
@@ -130,7 +131,7 @@ public class TaskThrottlerTest extends EasyMockTest {
         Optional.of(THROTTLED),
         PENDING,
         Optional.<String>absent()))
-        .andReturn(true);
+        .andReturn(StateChangeResult.SUCCESS);
   }
 
   private IScheduledTask makeTask(String id, ScheduleStatus status) {

http://git-wip-us.apache.org/repos/asf/aurora/blob/2dc1d59f/src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java
----------------------------------------------------------------------
diff --git a/src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java b/src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java
index 88fc172..b98a8d7 100644
--- a/src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java
+++ b/src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java
@@ -31,6 +31,7 @@ import org.apache.aurora.gen.ScheduledTask;
 import org.apache.aurora.gen.TaskConfig;
 import org.apache.aurora.gen.TaskEvent;
 import org.apache.aurora.scheduler.events.PubsubEvent.TaskStateChange;
+import org.apache.aurora.scheduler.state.StateChangeResult;
 import org.apache.aurora.scheduler.state.StateManager;
 import org.apache.aurora.scheduler.storage.entities.IScheduledTask;
 import org.apache.aurora.scheduler.storage.testing.StorageTestUtil;
@@ -143,7 +144,7 @@ public class TaskTimeoutTest extends EasyMockTest {
         Optional.of(KILLING),
         LOST,
         TaskTimeout.TIMEOUT_MESSAGE))
-        .andReturn(true);
+        .andReturn(StateChangeResult.SUCCESS);
 
     replayAndCreate();
 
@@ -161,7 +162,7 @@ public class TaskTimeoutTest extends EasyMockTest {
         Optional.of(ASSIGNED),
         LOST,
         TaskTimeout.TIMEOUT_MESSAGE))
-        .andReturn(true);
+        .andReturn(StateChangeResult.SUCCESS);
 
     replayAndCreate();
 
@@ -180,7 +181,7 @@ public class TaskTimeoutTest extends EasyMockTest {
         Optional.of(KILLING),
         LOST,
         TaskTimeout.TIMEOUT_MESSAGE))
-        .andReturn(false);
+        .andReturn(StateChangeResult.ILLEGAL);
 
     replayAndCreate();
 

http://git-wip-us.apache.org/repos/asf/aurora/blob/2dc1d59f/src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
----------------------------------------------------------------------
diff --git a/src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
b/src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
index 32d18a9..6ecdbd1 100644
--- a/src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
+++ b/src/test/java/org/apache/aurora/scheduler/async/preemptor/PreemptorImplTest.java
@@ -31,6 +31,7 @@ import org.apache.aurora.scheduler.async.OfferManager;
 import org.apache.aurora.scheduler.async.preemptor.Preemptor.PreemptorImpl;
 import org.apache.aurora.scheduler.base.TaskGroupKey;
 import org.apache.aurora.scheduler.base.Tasks;
+import org.apache.aurora.scheduler.state.StateChangeResult;
 import org.apache.aurora.scheduler.state.StateManager;
 import org.apache.aurora.scheduler.stats.CachedCounters;
 import org.apache.aurora.scheduler.storage.Storage;
@@ -155,7 +156,7 @@ public class PreemptorImplTest extends EasyMockTest {
         eq(Optional.<ScheduleStatus>absent()),
         eq(ScheduleStatus.PREEMPTING),
         EasyMock.<Optional<String>>anyObject()))
-        .andReturn(true);
+        .andReturn(StateChangeResult.SUCCESS);
   }
 
   private static PreemptionProposal createPreemptionProposal(IScheduledTask task) {

http://git-wip-us.apache.org/repos/asf/aurora/blob/2dc1d59f/src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java
----------------------------------------------------------------------
diff --git a/src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java
b/src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java
index 831803f..b9e1657 100644
--- a/src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java
+++ b/src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java
@@ -25,6 +25,7 @@ import org.apache.aurora.gen.AssignedTask;
 import org.apache.aurora.gen.CronCollisionPolicy;
 import org.apache.aurora.gen.ScheduleStatus;
 import org.apache.aurora.gen.ScheduledTask;
+import org.apache.aurora.scheduler.state.StateChangeResult;
 import org.apache.aurora.scheduler.state.StateManager;
 import org.apache.aurora.scheduler.storage.Storage;
 import org.apache.aurora.scheduler.storage.db.DbUtil;
@@ -123,7 +124,7 @@ public class AuroraCronJobTest extends EasyMockTest {
         eq(Optional.<ScheduleStatus>absent()),
         eq(ScheduleStatus.KILLING),
         eq(AuroraCronJob.KILL_AUDIT_MESSAGE)))
-        .andReturn(true);
+        .andReturn(StateChangeResult.SUCCESS);
     backoffHelper.doUntilSuccess(EasyMock.capture(capture));
     stateManager.insertPendingTasks(
         EasyMock.<MutableStoreProvider>anyObject(),

http://git-wip-us.apache.org/repos/asf/aurora/blob/2dc1d59f/src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java
----------------------------------------------------------------------
diff --git a/src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java
b/src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java
index 7b101bc..0d54049 100644
--- a/src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java
+++ b/src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java
@@ -193,7 +193,7 @@ public class MaintenanceControllerImplTest extends EasyMockTest {
         Optional.<ScheduleStatus>absent(),
         ScheduleStatus.DRAINING,
         MaintenanceControllerImpl.DRAINING_MESSAGE))
-        .andReturn(true);
+        .andReturn(StateChangeResult.SUCCESS);
   }
 
   private void expectFetchTasksByHost(String hostName, ImmutableSet<ScheduledTask>
tasks) {

http://git-wip-us.apache.org/repos/asf/aurora/blob/2dc1d59f/src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java
----------------------------------------------------------------------
diff --git a/src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java b/src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java
index 15e4d38..ff0ef02 100644
--- a/src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java
+++ b/src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java
@@ -69,12 +69,14 @@ import static org.apache.aurora.gen.ScheduleStatus.LOST;
 import static org.apache.aurora.gen.ScheduleStatus.PENDING;
 import static org.apache.aurora.gen.ScheduleStatus.RUNNING;
 import static org.apache.aurora.gen.ScheduleStatus.THROTTLED;
+import static org.apache.aurora.scheduler.state.StateChangeResult.ILLEGAL;
+import static org.apache.aurora.scheduler.state.StateChangeResult.INVALID_CAS_STATE;
+import static org.apache.aurora.scheduler.state.StateChangeResult.NOOP;
+import static org.apache.aurora.scheduler.state.StateChangeResult.SUCCESS;
 import static org.easymock.EasyMock.capture;
 import static org.easymock.EasyMock.expect;
 import static org.easymock.EasyMock.expectLastCall;
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
 
 public class StateManagerImplTest extends EasyMockTest {
 
@@ -216,8 +218,8 @@ public class StateManagerImplTest extends EasyMockTest {
     control.replay();
 
     insertTask(NON_SERVICE_CONFIG, 0);
-    assertEquals(true, changeState(taskId, KILLING));
-    assertEquals(false, changeState(taskId, KILLING));
+    assertEquals(SUCCESS, changeState(taskId, KILLING));
+    assertEquals(ILLEGAL, changeState(taskId, KILLING));
   }
 
   @Test
@@ -231,10 +233,10 @@ public class StateManagerImplTest extends EasyMockTest {
 
     insertTask(NON_SERVICE_CONFIG, 0);
     assignTask(taskId, HOST_A);
-    assertEquals(true, changeState(taskId, RUNNING));
-    assertEquals(true, changeState(taskId, KILLING));
-    assertEquals(true, changeState(taskId, KILLED));
-    assertEquals(false, changeState(taskId, KILLED));
+    assertEquals(SUCCESS, changeState(taskId, RUNNING));
+    assertEquals(SUCCESS, changeState(taskId, KILLING));
+    assertEquals(SUCCESS, changeState(taskId, KILLED));
+    assertEquals(NOOP, changeState(taskId, KILLED));
   }
 
   @Test
@@ -370,12 +372,12 @@ public class StateManagerImplTest extends EasyMockTest {
 
     insertTask(config, 0);
     assignTask(taskId, HOST_A);
-    assertFalse(changeState(
+    assertEquals(INVALID_CAS_STATE, changeState(
         taskId,
         Optional.of(PENDING),
         RUNNING,
         Optional.<String>absent()));
-    assertTrue(changeState(
+    assertEquals(SUCCESS, changeState(
         taskId,
         Optional.of(ASSIGNED),
         FAILED,
@@ -386,7 +388,7 @@ public class StateManagerImplTest extends EasyMockTest {
   public void testCasTaskNotFound() {
     control.replay();
 
-    assertFalse(changeState(
+    assertEquals(INVALID_CAS_STATE, changeState(
         "a",
         Optional.of(PENDING),
         ASSIGNED,
@@ -551,15 +553,15 @@ public class StateManagerImplTest extends EasyMockTest {
     });
   }
 
-  private boolean changeState(
+  private StateChangeResult changeState(
       final String taskId,
       final Optional<ScheduleStatus> casState,
       final ScheduleStatus newState,
       final Optional<String> auditMessage) {
 
-    return storage.write(new Storage.MutateWork.Quiet<Boolean>() {
+    return storage.write(new Storage.MutateWork.Quiet<StateChangeResult>() {
       @Override
-      public Boolean apply(Storage.MutableStoreProvider storeProvider) {
+      public StateChangeResult apply(Storage.MutableStoreProvider storeProvider) {
         return stateManager.changeState(
             storeProvider,
             taskId,
@@ -570,7 +572,7 @@ public class StateManagerImplTest extends EasyMockTest {
     });
   }
 
-  private boolean changeState(final String taskId, final ScheduleStatus status) {
+  private StateChangeResult changeState(final String taskId, final ScheduleStatus status)
{
     return changeState(
         taskId,
         Optional.<ScheduleStatus>absent(),

http://git-wip-us.apache.org/repos/asf/aurora/blob/2dc1d59f/src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java
----------------------------------------------------------------------
diff --git a/src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java b/src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java
index afbca61..b7326a6 100644
--- a/src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java
+++ b/src/test/java/org/apache/aurora/scheduler/state/TaskStateMachineTest.java
@@ -37,6 +37,10 @@ import org.apache.aurora.scheduler.storage.entities.IScheduledTask;
 import org.junit.Before;
 import org.junit.Test;
 
+import static org.apache.aurora.scheduler.state.StateChangeResult.ILLEGAL;
+import static org.apache.aurora.scheduler.state.StateChangeResult.ILLEGAL_WITH_SIDE_EFFECTS;
+import static org.apache.aurora.scheduler.state.StateChangeResult.NOOP;
+import static org.apache.aurora.scheduler.state.StateChangeResult.SUCCESS;
 import static org.apache.aurora.scheduler.state.TaskStateMachine.TaskState.ASSIGNED;
 import static org.apache.aurora.scheduler.state.TaskStateMachine.TaskState.DELETED;
 import static org.apache.aurora.scheduler.state.TaskStateMachine.TaskState.DRAINING;
@@ -53,9 +57,7 @@ import static org.apache.aurora.scheduler.state.TaskStateMachine.TaskState.RUNNI
 import static org.apache.aurora.scheduler.state.TaskStateMachine.TaskState.STARTING;
 import static org.apache.aurora.scheduler.state.TaskStateMachine.TaskState.THROTTLED;
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotEquals;
-import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 // TODO(wfarner): At this rate, it's probably best to exhaustively cover this class with
a matrix
@@ -117,7 +119,11 @@ public class TaskStateMachineTest {
       legalTransition(TaskState.valueOf(endState.name()), finalActions);
 
       for (ScheduleStatus badTransition : Tasks.TERMINAL_STATES) {
-        illegalTransition(TaskState.valueOf(badTransition.name()));
+        if (endState == badTransition) {
+          assertEquals(NOOP, stateMachine.updateState(Optional.of(badTransition)).getResult());
+        } else {
+          illegalTransition(TaskState.valueOf(badTransition.name()));
+        }
       }
     }
   }
@@ -284,7 +290,7 @@ public class TaskStateMachineTest {
   private void legalTransition(TaskState state, Set<SideEffect.Action> expectedActions)
{
     ScheduleStatus previousState = stateMachine.getPreviousState();
     TransitionResult result = stateMachine.updateState(state.getStatus());
-    assertTrue("Transition to " + state + " was not successful", result.isSuccess());
+    assertEquals("Transition to " + state + " was not successful", SUCCESS, result.getResult());
     assertNotEquals(previousState, stateMachine.getPreviousState());
     assertEquals(
         FluentIterable.from(expectedActions).transform(TO_SIDE_EFFECT).toSet(),
@@ -305,9 +311,10 @@ public class TaskStateMachineTest {
   }
 
   private void illegalTransition(TaskState state, Set<SideEffect> sideEffects) {
-    TransitionResult result = stateMachine.updateState(state.getStatus());
-    assertFalse(result.isSuccess());
-    assertEquals(sideEffects, result.getSideEffects());
+    TransitionResult expected = new TransitionResult(
+        sideEffects.isEmpty() ? ILLEGAL : ILLEGAL_WITH_SIDE_EFFECTS,
+        ImmutableSet.copyOf(sideEffects));
+    assertEquals(expected, stateMachine.updateState(state.getStatus()));
   }
 
   private static ScheduledTask makeTask(boolean service) {
@@ -324,34 +331,34 @@ public class TaskStateMachineTest {
   }
 
   private static final TransitionResult SAVE = new TransitionResult(
-      true,
+      SUCCESS,
       ImmutableSet.of(new SideEffect(Action.SAVE_STATE, Optional.<ScheduleStatus>absent())));
   private static final TransitionResult SAVE_AND_KILL = new TransitionResult(
-      true,
+      SUCCESS,
       ImmutableSet.of(
           new SideEffect(Action.SAVE_STATE, Optional.<ScheduleStatus>absent()),
           new SideEffect(Action.KILL, Optional.<ScheduleStatus>absent())));
   private static final TransitionResult SAVE_AND_RESCHEDULE = new TransitionResult(
-      true,
+      SUCCESS,
       ImmutableSet.of(
           new SideEffect(Action.SAVE_STATE, Optional.<ScheduleStatus>absent()),
           new SideEffect(Action.RESCHEDULE, Optional.<ScheduleStatus>absent())));
   private static final TransitionResult SAVE_KILL_AND_RESCHEDULE = new TransitionResult(
-      true,
+      SUCCESS,
       ImmutableSet.of(
           new SideEffect(Action.SAVE_STATE, Optional.<ScheduleStatus>absent()),
           new SideEffect(Action.KILL, Optional.<ScheduleStatus>absent()),
           new SideEffect(Action.RESCHEDULE, Optional.<ScheduleStatus>absent())));
   private static final TransitionResult ILLEGAL_KILL = new TransitionResult(
-      false,
+      ILLEGAL_WITH_SIDE_EFFECTS,
       ImmutableSet.of(new SideEffect(Action.KILL, Optional.<ScheduleStatus>absent())));
   private static final TransitionResult RECORD_FAILURE = new TransitionResult(
-      true,
+      SUCCESS,
       ImmutableSet.of(
           new SideEffect(Action.SAVE_STATE, Optional.<ScheduleStatus>absent()),
           new SideEffect(Action.INCREMENT_FAILURES, Optional.<ScheduleStatus>absent())));
   private static final TransitionResult DELETE_TASK = new TransitionResult(
-      true,
+      SUCCESS,
       ImmutableSet.of(new SideEffect(Action.DELETE, Optional.<ScheduleStatus>absent())));
 
   private static final class TestCase {
@@ -531,7 +538,11 @@ public class TaskStateMachineTest {
 
           TransitionResult expectation = EXPECTATIONS.get(testCase);
           if (expectation == null) {
-            expectation = new TransitionResult(false, ImmutableSet.<SideEffect>of());
+            if (taskPresent && from == to || !taskPresent && to == DELETED)
{
+              expectation = new TransitionResult(NOOP, ImmutableSet.<SideEffect>of());
+            } else {
+              expectation = new TransitionResult(ILLEGAL, ImmutableSet.<SideEffect>of());
+            }
           }
 
           TaskStateMachine machine;

http://git-wip-us.apache.org/repos/asf/aurora/blob/2dc1d59f/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
----------------------------------------------------------------------
diff --git a/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
b/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
index 1ac1a28..21b4044 100644
--- a/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
+++ b/src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
@@ -92,6 +92,7 @@ import org.apache.aurora.scheduler.quota.QuotaManager;
 import org.apache.aurora.scheduler.state.LockManager;
 import org.apache.aurora.scheduler.state.LockManager.LockException;
 import org.apache.aurora.scheduler.state.MaintenanceController;
+import org.apache.aurora.scheduler.state.StateChangeResult;
 import org.apache.aurora.scheduler.state.StateManager;
 import org.apache.aurora.scheduler.state.UUIDGenerator;
 import org.apache.aurora.scheduler.storage.Storage.StorageException;
@@ -655,7 +656,7 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest {
         TASK_ID,
         Optional.<ScheduleStatus>absent(),
         ScheduleStatus.KILLING,
-        killedByMessage(USER))).andReturn(true);
+        killedByMessage(USER))).andReturn(StateChangeResult.SUCCESS);
   }
 
   @Test
@@ -885,7 +886,7 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest {
         TASK_ID,
         Optional.<ScheduleStatus>absent(),
         ScheduleStatus.FAILED,
-        Optional.of(transitionMessage(USER).get()))).andReturn(true);
+        Optional.of(transitionMessage(USER).get()))).andReturn(StateChangeResult.SUCCESS);
 
     expectAuth(ROOT, true);
     expectAuth(ROOT, false);
@@ -983,7 +984,7 @@ public class SchedulerThriftInterfaceTest extends EasyMockTest {
         TASK_ID,
         Optional.<ScheduleStatus>absent(),
         ScheduleStatus.RESTARTING,
-        restartedByMessage(USER))).andReturn(true);
+        restartedByMessage(USER))).andReturn(StateChangeResult.SUCCESS);
 
     control.replay();
 

http://git-wip-us.apache.org/repos/asf/aurora/blob/2dc1d59f/src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java
----------------------------------------------------------------------
diff --git a/src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java b/src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java
index 4e7ff3b..7aa19d4 100644
--- a/src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java
+++ b/src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java
@@ -69,6 +69,7 @@ import org.apache.aurora.scheduler.events.PubsubEvent;
 import org.apache.aurora.scheduler.mesos.Driver;
 import org.apache.aurora.scheduler.state.LockManager;
 import org.apache.aurora.scheduler.state.LockManagerImpl;
+import org.apache.aurora.scheduler.state.StateChangeResult;
 import org.apache.aurora.scheduler.state.StateManager;
 import org.apache.aurora.scheduler.state.StateManagerImpl;
 import org.apache.aurora.scheduler.state.UUIDGenerator;
@@ -125,7 +126,6 @@ import static org.apache.aurora.scheduler.storage.Storage.MutateWork.NoResult;
 import static org.apache.aurora.scheduler.updater.UpdateFactory.UpdateFactoryImpl.expandInstanceIds;
 import static org.easymock.EasyMock.expectLastCall;
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 public class JobUpdaterIT extends EasyMockTest {
@@ -242,7 +242,7 @@ public class JobUpdaterIT extends EasyMockTest {
       storage.write(new NoResult.Quiet() {
         @Override
         protected void execute(Storage.MutableStoreProvider storeProvider) {
-          assertTrue(stateManager.changeState(
+          assertEquals(StateChangeResult.SUCCESS, stateManager.changeState(
               storeProvider,
               getTaskId(job, instanceId),
               Optional.<ScheduleStatus>absent(),


Mime
View raw message