[REEF-1447] Validate Task close failure => FailedEvaluator Event (task has… #1042

afchung · 2016-06-10T00:11:50Z

… not yet finished)

This addressed the issue by

Failing the Evaluator on an Exception in TaskCloseHandler.
Adding a test to test for close event failure while Task is still running. Validating that the TaskCloseHandler Exception triggers a FailedEvaluator.
Modify TaskRuntime and related tests to verify that the TaskStopHandler is not called upon Task failure.

markusweimer · 2016-06-10T16:24:46Z

lang/cs/Org.Apache.REEF.Common/Runtime/Evaluator/Task/TaskRuntime.cs

@@ -110,9 +110,9 @@ public Thread StartTaskOnNewThread()
                            "Task running result:\r\n" + System.Text.Encoding.Default.GetString(result));
                    }
                }
-                catch (TaskStopHandlerException e)
+                catch (TaskStopHandlerException stopException)


I appreciate the clarification. But this is a pure syntactic change, maybe leave it out of this PR?

markusweimer · 2016-06-10T16:27:43Z

I've done a pass.

afchung · 2016-06-10T16:41:18Z

@markusweimer replied to your comments. The current design only reports the earliest Exception to the user. In terms of when we encounter multiple Exception s, which idea seems better? Would like your input before implementing the next iteration. Thanks!

afchung · 2016-06-14T16:49:13Z

@markusweimer what are your thoughts on how to handle multiple Exceptions. Should we just log and ignore? Or should we create AggregateExceptions?

jwang98052 · 2016-06-15T03:14:33Z

lang/cs/Org.Apache.REEF.Common/Runtime/Evaluator/Task/TaskStatus.cs

@@ -98,28 +98,48 @@ public void SetException(Exception e)
        {
            lock (_heartBeatManager)
            {
-                try
+                if (_lastException.IsPresent())


Is this to ensure failed task is only send once?

jwang98052 · 2016-06-15T18:01:45Z

I have done a pass.

afchung · 2016-06-17T20:12:01Z

@jwang98052 @markusweimer I'll resolve the conflict once we reach a consensus on how to handle multiple Exceptions on a failed Task. Thanks!

andrey-me · 2016-06-20T23:44:00Z

lang/cs/Org.Apache.REEF.Common/Runtime/Evaluator/Task/TaskStatus.cs

+                {
+                    if (!_lastException.IsPresent())
+                    {
+                        throw new ApplicationException("Expected failed Task to already have set an Exception.");


Looks like this is optional in SetException...do we really want to generate new exception here?
To clarify, it seem to check for integrity of the Failed state in wrong place: If we want to fast fail we should fail in SetException().

We only reach _state = Failed in SetException, and SetException has to set _lastException to a "present" Optional. Everything is also done with a lock held on _heartbeatManager. Thus, if we ever reach a state where _state == Failed but _lastException is not "present", we must have had some sort of application logic and we should not call SetException again. Instead, we should crash the Evaluator with an ApplicationException that is not caught in the upper layers.

right, can we crash evaluator in the SetException right away?
Basically if we want to guarantee state integrity, it is easier to do during state change, otherwise we may need to do these checks in multiply places afterwards.

In reply to: 67907809 [](ancestors = 67907809)

afchung · 2016-06-21T22:00:20Z

@jwang98052 @markusweimer @andrey-me This is ready for another round. Thanks!

@jwang98052 note that we have changed the behavior of an Exception occurring in TaskCloseHandler. This may change the behavior in fault tolerance, when you are forcing Tasks to close. I've modified the tests to fit the behavior, but there might be things I'm missing. Please take a closer look. Thanks!

afchung · 2016-06-21T22:02:25Z

@jwang98052 for the motivation of the change, please read the JIRA. Thanks!

jwang98052 · 2016-06-22T00:30:51Z

It does break the current implementation of IMRU fault tolerant. I have opened REEF-1466 for the solution/discussion.

markusweimer · 2016-06-23T18:19:19Z

It does break the current implementation of IMRU fault tolerant. I have opened REEF-1466 for the solution/discussion.

@jwang98052 As discussed over on JIRA, the reliance on exceptions thrown by a StopHandler to stop a Task does not work, regardless of whether or not this gets merged. Hence, shall we go ahead with this change?

markusweimer · 2016-06-23T18:21:31Z

lang/cs/Org.Apache.REEF.Common/Runtime/Evaluator/Task/TaskRuntime.cs

@@ -41,6 +41,7 @@ internal sealed class TaskRuntime : IObserver<ICloseEvent>, IObserver<ISuspendEv
        private readonly IInjectionFuture<IObserver<ISuspendEvent>> _suspendHandlerFuture;
        private readonly IInjectionFuture<IObserver<ICloseEvent>> _closeHandlerFuture;
        private int _taskRan = 0;
+        private int _taskClosed = 0;


Shouldn't these be bool?

bool does not work with Interlocked.

markusweimer · 2016-06-23T18:30:09Z

I've done another pass.

jwang98052 · 2016-06-24T21:04:20Z

lang/cs/Org.Apache.REEF.Common/Runtime/Evaluator/Task/TaskStatus.cs

-                    }
-
-                    State = TaskState.Failed;
-                    _taskLifeCycle.Stop();


So if task exception happens, don't call TaskStopHandler, right? In another word, TaskStopHandler is only for happen scenario, right?

jwang98052 · 2016-06-24T22:10:50Z

@markusweimer After REEF-1466 is resolved, we should be fine with this change. In REEF-1466, we will use cancellation token to let the task return properly from close handler instead of throw exception.
@afchung with this change, I would expect IMRUCloseTaskTest fail as it throws exception in task close handler and expecting FailedTask if the task cannot return properly. I don't know how it passed test somehow. I have updated this test in REEF-1466.

afchung · 2016-06-24T23:40:50Z

@markusweimer @jwang98052 I've addressed your comments, please have another look. Thanks!

afchung · 2016-06-27T20:07:04Z

@jwang98052 is this ready to be merged? I've updated IMRUCloseTaskTest in this PR to handle the failed Evaluator scenario, that is why it is passing.

jwang98052 · 2016-06-27T22:56:48Z

@afchung I see you modified TaskCloseTest, but didn't find the change for IMRUCloseTaskTest. Did I miss it?

afchung · 2016-06-27T23:08:42Z

@jwang98052 it seems like I didn't need to change it since the Tasks always complete. My understanding taking a glance as the code is that what the test does is it just runs broadcast and reduce without injecting any failure and hence there is no need for us to close the Tasks. Given that TestBroadcastAndReduce always runs until the Driver is completed, we always wait for all Tasks to finish.

afchung · 2016-06-27T23:09:04Z

We should probably change that test to include deterministic, injected failures.

jwang98052 · 2016-06-27T23:24:36Z

@afchung The test send close event to every task. First task should close with CompletedTask, others, in most cases will stuck in data reading and eventually exception will be throw. With old behavior, it should end up to FailedTask. With new behavior, it will end up to FailedEvaluator, Now they all end up to CompletedTask, that is why I don't understand. Looks like they are all completed before receiving close event.

I have changed it with REEF-1466, the behavior will be deterministic.

afchung · 2016-06-27T23:25:45Z

Should we merge REEF-1466 first then? What do you suggest? I'm not familiar with the test so I will not be modifying it.

jwang98052 · 2016-06-27T23:29:33Z

Theoretically, we should merge REEF-1469 first, I will then need to update REEF-1466 to resolve the conflict. Then REEF-1447. But if you have any dependency on it, we might want to move quicker as you have limited days left and I will ensure this test work eventually.

afchung · 2016-06-27T23:32:49Z

I have a few more tests that depend on classes in this PR (see #1057). Alternatively we can try to merge #1057 first. Either way I would like to get WaitingTask and EventHandle into the main codebase first.

jwang98052 · 2016-06-27T23:39:43Z

I will look #1057 as early as I can. Would it conflict with this one #1042?

afchung · 2016-06-28T16:37:22Z

Yes, it will conflict with this PR. I'll merge if conflict occurs. Thanks!

… not yet finished) This addressed the issue by * Failing the Evaluator on an `Exception` in `TaskCloseHandler`. * Adding a test to test for close event failure while Task is still running. Validating that the `TaskCloseHandler` `Exception` triggers a `FailedEvaluator`. * Modify `TaskRuntime` and related tests to verify that the `TaskStopHandler` is not called upon `Task` failure. JIRA: [REEF-1447](https://issues.apache.org/jira/browse/REEF-1447)

afchung · 2016-07-01T18:51:05Z

@jwang98052 @markusweimer I've resolved the conflict and rebased. Thanks!

jwang98052 · 2016-07-01T20:51:30Z

I will test and merge.

markusweimer reviewed Jun 10, 2016
View reviewed changes

jwang98052 reviewed Jun 15, 2016
View reviewed changes

andrey-me reviewed Jun 20, 2016
View reviewed changes

afchung changed the title ~~[REEF-1447] Validate Task close failure => FailedTask Event (task has…~~ [REEF-1447] Validate Task close failure => FailedEvaluator Event (task has… Jun 21, 2016

afchung force-pushed the REEF-1447 branch from 1d06f62 to a1246e6 Compare June 21, 2016 21:58

markusweimer reviewed Jun 23, 2016
View reviewed changes

jwang98052 reviewed Jun 24, 2016
View reviewed changes

afchung mentioned this pull request Jun 27, 2016

[REEF-1431] Validate Task Message Receive failure => FailedEvaluator … #1057

Closed

afchung force-pushed the REEF-1447 branch from ec5f9e2 to 1909ad2 Compare July 1, 2016 18:50

asfgit closed this in 4ab6a8d Jul 1, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REEF-1447] Validate Task close failure => FailedEvaluator Event (task has… #1042

[REEF-1447] Validate Task close failure => FailedEvaluator Event (task has… #1042

afchung commented Jun 10, 2016 •

edited

markusweimer Jun 10, 2016

markusweimer commented Jun 10, 2016

afchung commented Jun 10, 2016

afchung commented Jun 14, 2016

jwang98052 Jun 15, 2016

afchung Jun 15, 2016

jwang98052 commented Jun 15, 2016

afchung commented Jun 17, 2016

andrey-me Jun 20, 2016 •

edited

afchung Jun 21, 2016

andrey-me Jun 21, 2016 •

edited

afchung commented Jun 21, 2016

afchung commented Jun 21, 2016

jwang98052 commented Jun 22, 2016

markusweimer commented Jun 23, 2016

markusweimer Jun 23, 2016

afchung Jun 24, 2016

markusweimer commented Jun 23, 2016

jwang98052 Jun 24, 2016 •

edited

afchung Jun 24, 2016

jwang98052 commented Jun 24, 2016

afchung commented Jun 24, 2016

afchung commented Jun 27, 2016

jwang98052 commented Jun 27, 2016

afchung commented Jun 27, 2016

afchung commented Jun 27, 2016

jwang98052 commented Jun 27, 2016

afchung commented Jun 27, 2016

jwang98052 commented Jun 27, 2016

afchung commented Jun 27, 2016

jwang98052 commented Jun 27, 2016

afchung commented Jun 28, 2016

afchung commented Jul 1, 2016

jwang98052 commented Jul 1, 2016

[REEF-1447] Validate Task close failure => FailedEvaluator Event (task has… #1042

[REEF-1447] Validate Task close failure => FailedEvaluator Event (task has… #1042

Conversation

afchung commented Jun 10, 2016 • edited

markusweimer Jun 10, 2016

Choose a reason for hiding this comment

markusweimer commented Jun 10, 2016

afchung commented Jun 10, 2016

afchung commented Jun 14, 2016

jwang98052 Jun 15, 2016

Choose a reason for hiding this comment

afchung Jun 15, 2016

Choose a reason for hiding this comment

jwang98052 commented Jun 15, 2016

afchung commented Jun 17, 2016

andrey-me Jun 20, 2016 • edited

Choose a reason for hiding this comment

afchung Jun 21, 2016

Choose a reason for hiding this comment

andrey-me Jun 21, 2016 • edited

Choose a reason for hiding this comment

afchung commented Jun 21, 2016

afchung commented Jun 21, 2016

jwang98052 commented Jun 22, 2016

markusweimer commented Jun 23, 2016

markusweimer Jun 23, 2016

Choose a reason for hiding this comment

afchung Jun 24, 2016

Choose a reason for hiding this comment

markusweimer commented Jun 23, 2016

jwang98052 Jun 24, 2016 • edited

Choose a reason for hiding this comment

afchung Jun 24, 2016

Choose a reason for hiding this comment

jwang98052 commented Jun 24, 2016

afchung commented Jun 24, 2016

afchung commented Jun 27, 2016

jwang98052 commented Jun 27, 2016

afchung commented Jun 27, 2016

afchung commented Jun 27, 2016

jwang98052 commented Jun 27, 2016

afchung commented Jun 27, 2016

jwang98052 commented Jun 27, 2016

afchung commented Jun 27, 2016

jwang98052 commented Jun 27, 2016

afchung commented Jun 28, 2016

afchung commented Jul 1, 2016

jwang98052 commented Jul 1, 2016

afchung commented Jun 10, 2016 •

edited

andrey-me Jun 20, 2016 •

edited

andrey-me Jun 21, 2016 •

edited

jwang98052 Jun 24, 2016 •

edited