Adds max running time termination criteria #474

jeppe-style · 2017-07-02T15:59:33Z

as per title

Adds method to terminate experiment in task scheduler
Refactors running states handling to separate class
Adds morphia converter for Time

fixes #337 (except extensions, that are now in #517)

related to #281

depends on PR #469

TODO

issues with calling wrong function: TestTaskScheduler.java#L273, TestTaskScheduler.java#L281
issues that not all states (except running) check if test is terminated.

Adds method to terminate experiment in task scheduler. Refactors running states handling to separate class. Adds morphia converter for Time.

VincenzoFerme · 2017-07-10T13:58:58Z

...r/application/src/main/java/cloud/benchflow/testmanager/BenchFlowTestManagerApplication.java

@@ -127,6 +128,8 @@ public void run(BenchFlowTestManagerConfiguration configuration, Environment env
    // http://mongodb.github.io/mongo-java-driver/3.4/driver/getting-started/quick-start/
    MongoClient mongoClient = configuration.getMongoDBFactory().build();
    ExecutorService taskExecutor = configuration.getTaskExecutorFactory().build(environment);
+    ScheduledThreadPoolExecutor timeOutScheduledThreadPoolExecutor =


You should build it as you do for the other executor service we have: https://github.com/benchflow/benchflow/pull/474/files#diff-3ebdba5e745234429a46f7536fa3feb8R130

Why? I don't see any need to put this in the configuration

Postponed in #517

VincenzoFerme · 2017-07-10T14:07:29Z

...nager/application/src/main/java/cloud/benchflow/testmanager/scheduler/TestTaskScheduler.java

          break;

        case VALIDATE_TERMINATION_CRITERIA:
-          validateTerminationCriteria(testID);
+          runningStatesHandler.validateTerminationCriteria(testID);
          break;

        case DERIVE_PREDICTION_FUNCTION:
          testModelDAO.setTestRunningState(testID, VALIDATE_PREDICTION_FUNCTION);


Why you do this here and not in the function called in the next line?

It actually happens also happens in the function in the next line. Although this wasn't changed in this PR.

Fixed in #474 (commits)

VincenzoFerme · 2017-07-10T14:09:38Z

...nager/application/src/main/java/cloud/benchflow/testmanager/scheduler/TestTaskScheduler.java

@@ -135,6 +135,11 @@ private synchronized void handleStartState(String testID) {
      // wait for task to complete
      future.get();

+      if (isTerminated(testID)) {


Why don't you check this in the setTestState method, so that is done only in one place?

Anyway this should not be possible, because once you get a timeout, you need to get exclusive access to the data structure where you keep running information and cleanup everything before other code gets executed.

The running tasks cannot be cancelled - the only way is to wait for them to terminate. Therefore I need to check if the test has been terminated in the meantime. It is also what we discussed.

Fixed in #474 (commits)

VincenzoFerme · 2017-07-10T14:10:43Z

...nager/application/src/main/java/cloud/benchflow/testmanager/scheduler/TestTaskScheduler.java

@@ -156,6 +161,18 @@ private synchronized void handleWaitingState(String testID) {
      // set state as ready
      testModelDAO.setTestState(testID, BenchFlowTestState.READY);

+      // update max running time timeout


Why do you need the following code? Is it to restart the execution, and take into account of the time the test has already been executed? If yes, I think this should happen in the RUNNING state when the test goes back to RUNNING.

I need to update the maxRunning time since that is read when the test goes back to RUNNING. In the WAITING state that is the only place I know how much time has elapsed since the TimeoutTask was scheduled.

VincenzoFerme · 2017-07-10T14:15:32Z

...nager/application/src/main/java/cloud/benchflow/testmanager/scheduler/TestTaskScheduler.java

@@ -172,44 +189,51 @@ private synchronized void handleTestRunningState(String testID) {
      logger
          .info("handleTestRunningState for " + testID + " with state " + testRunningState.name());

+      // set timeout if not already set
+      if (!timeoutTasks.containsKey(testID)) {


What happens if the test has no timeout?

I don't understand the question fully

Fixed in #474 (commits)

VincenzoFerme · 2017-07-10T17:06:11Z

...cation/src/main/java/cloud/benchflow/testmanager/scheduler/running/RunningStatesHandler.java

+    try {
+
+      // wait for task to complete
+      future.get();


If you make task cancellable, then probably here you are going to get an exception and you follow the exceptional flow, so that you do not need to check all the time if something is terminated. When you apply the previous comment, then the handling can be delegated to a method.

see previous comment about cancellable tasks.

Worked on this in #474 (commits). It is fine to have.

VincenzoFerme · 2017-07-10T17:12:21Z

...cation/src/main/java/cloud/benchflow/testmanager/scheduler/running/RunningStatesHandler.java

+    // replace with new task
+    testTasks.put(testID, future);
+
+    // we don't wait for the task to complete since the experiment-manager


Probably at this stage, the waiting for this state should be exactly to wait for the wanted data from the experiment manager, that should be the details for each of the trials that actually gets executed up to the point in which the experiment manager declares the experiment as completed in any of the possible states.

this is what happens - the EM informs when the experiment has completed

Worked on this in #474 (commits).

VincenzoFerme · 2017-07-10T17:13:08Z

...cation/src/main/java/cloud/benchflow/testmanager/scheduler/running/RunningStatesHandler.java

+
+    try {
+
+      // TODO - update: set next state as validate termination criteria


You set it to REMOVE_NON_REACHABLE_EXPERIMENTS

Not sure what the comment is about. This is a TODO

VincenzoFerme · 2017-07-10T17:20:27Z

.../application/src/main/java/cloud/benchflow/testmanager/tasks/abort/AbortRunningTestTask.java

+      String experimentID = testModelDAO.getRunningExperiment(testID);
+
+      if (experimentID != null) {
+        experimentManagerService.abortBenchFlowExperiment(experimentID);


You might want to get and ACK about the execution of the operation, because otherwise it is really complex to control the behaviour of the distributed system.

I think this is covered in issue #499

VincenzoFerme · 2017-07-10T17:22:59Z

...ger/application/src/test/java/cloud/benchflow/testmanager/scheduler/TestTaskSchedulerIT.java

@@ -267,6 +270,60 @@ public void runLoadTest() throws Exception {

  }

+  @Test
+  public void runBenchFlowTestTimeoutTest() throws Exception {


You need also some tests where you submit multiple tests, and check the results of some of them, so that you can experience what happens with concurrency

added that to the issue #499

jeppe-style added benchflow-experiment-manager status-pr/blocked by p.r. feature labels Jul 2, 2017

jeppe-style added this to the Priorities milestone Jul 2, 2017

jeppe-style assigned VincenzoFerme Jul 2, 2017

jeppe-style requested a review from VincenzoFerme July 2, 2017 15:59

jeppe-style mentioned this pull request Jul 2, 2017

Demo June 2017 #403

Open

42 tasks

jeppe-style added benchflow-test-manager and removed benchflow-experiment-manager labels Jul 2, 2017

jeppe-style mentioned this pull request Jul 4, 2017

Saves exploration space in db #476

Merged

jeppe-style added the status/in progress label Jul 5, 2017

VincenzoFerme changed the base branch from devel to feature-tm-dsl-integration July 6, 2017 12:37

VincenzoFerme changed the base branch from feature-tm-dsl-integration to feature-tm-links-response-objects July 6, 2017 12:54

VincenzoFerme force-pushed the feature-tm-links-response-objects branch from b765c0d to 5aff9fd Compare July 6, 2017 13:29

jeppe-style removed the status/in progress label Jul 6, 2017

jeppe-style added 3 commits July 6, 2017 21:17

Adds max running time termination criteria

56ec3ca

Adds method to terminate experiment in task scheduler. Refactors running states handling to separate class. Adds morphia converter for Time.

Fixes bug in TestTaskScheduler

93f8274

Adds check for isTerminated for all states

798c420

jeppe-style force-pushed the feature-tm-test-termination-criteria branch 2 times, most recently from bdd5661 to 798c420 Compare July 6, 2017 19:31

jeppe-style mentioned this pull request Jul 7, 2017

Termination Criteria: To Be Implemented and Impact on Selection Strategies #281

Open

VincenzoFerme requested changes Jul 10, 2017

View reviewed changes

VincenzoFerme added the status-pr/needs changes label Jul 10, 2017

VincenzoFerme assigned jeppe-style and unassigned VincenzoFerme Jul 10, 2017

VincenzoFerme mentioned this pull request Jul 10, 2017

Adds abort experiment API #479

Merged

jeppe-style mentioned this pull request Jul 27, 2017

Saves exploration space in db #512

Closed

VincenzoFerme mentioned this pull request Jul 27, 2017

Saves exploration space in db #514

Merged

VincenzoFerme removed the status-pr/blocked by p.r. label Aug 3, 2017

VincenzoFerme changed the base branch from feature-tm-links-response-objects to devel August 3, 2017 13:43

VincenzoFerme added 17 commits August 8, 2017 16:23

Bumps Dropwizard, Mockito and Guava Versions

08d26a0

Marks Mockito and Docker Compose Rule as Test Dependencies

62bd17f

Reorganizes Imports for Test

3b0360e

Refactors Contansts in a Package

0490970

Adds Accessors for Testing

62c9dd8

Adds a Method to Check if a Test has Max Running Time

3e3ba79

Improves Tests for Fine Grain Control of Tasks Execution

48e9554

Adds Helpers to Wait for Tests Execution

39719da

Adds Method to Check if hasMaxRunningTime and Improves State Check

417469d

Adds Abortable Runnable, Callable and FutureTask

cde1bd5

Uses Abortable Runnable and Callable

8f99e44

Adds a Custom Executor Handling Abortable Tasks

4005245

Applies Code Formatting

6864ade

Uses the Custom Executor

200baaa

Formats the code of Scheduler and Helpers Tests

bad60d3

Improves Test Lifecycle and Termination Handling

04ab46d

Updates Resources to the New Test Lifecycle

73a4257

This was referenced Aug 8, 2017

Add a Different TestTerminatedState when Aborted in START or READY State #518

Open

Test Life Cycle TERMINATING State #499

Closed

VincenzoFerme and others added 6 commits August 8, 2017 19:01

Adds a Clarification Comment

7106c9d

Merge branch 'devel' into feature-tm-test-termination-criteria

5467e38

Fixes PMD Errors

5ea3cc1

Fixes Most of the Codacy Errors

4fff109

Fixes Maven JavaDoc Errors

275c92b

Deletes System Out

4849bb9

VincenzoFerme removed the status-pr/needs changes label Aug 9, 2017

VincenzoFerme merged commit a19d162 into devel Aug 9, 2017

VincenzoFerme deleted the feature-tm-test-termination-criteria branch August 9, 2017 07:43

This was referenced Aug 14, 2017

Improve how we wait for an experiment/test to finish in tests #395

Open

Add End-to-end Tests #498

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds max running time termination criteria #474

Adds max running time termination criteria #474

jeppe-style commented Jul 2, 2017 •

edited by VincenzoFerme

Loading

VincenzoFerme Jul 10, 2017

jeppe-style Jul 19, 2017

VincenzoFerme Aug 3, 2017

VincenzoFerme Jul 10, 2017

jeppe-style Jul 19, 2017

VincenzoFerme Aug 8, 2017

VincenzoFerme Jul 10, 2017

jeppe-style Jul 19, 2017

VincenzoFerme Aug 8, 2017

VincenzoFerme Jul 10, 2017

jeppe-style Jul 19, 2017

VincenzoFerme Jul 10, 2017

jeppe-style Jul 19, 2017

VincenzoFerme Aug 8, 2017

VincenzoFerme Jul 10, 2017

jeppe-style Jul 19, 2017

VincenzoFerme Aug 8, 2017

VincenzoFerme Jul 10, 2017

jeppe-style Jul 19, 2017

VincenzoFerme Aug 8, 2017 •

edited

Loading

VincenzoFerme Jul 10, 2017

jeppe-style Jul 19, 2017

VincenzoFerme Jul 10, 2017

jeppe-style Jul 19, 2017

VincenzoFerme Jul 10, 2017

jeppe-style Jul 19, 2017


		try {

		// TODO - update: set next state as validate termination criteria

Adds max running time termination criteria #474

Adds max running time termination criteria #474

Conversation

jeppe-style commented Jul 2, 2017 • edited by VincenzoFerme Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincenzoFerme Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeppe-style commented Jul 2, 2017 •

edited by VincenzoFerme

Loading

VincenzoFerme Aug 8, 2017 •

edited

Loading