Skip to content
This repository has been archived by the owner on Aug 4, 2022. It is now read-only.

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

Closed
wants to merge 2 commits into from

Conversation

afchung
Copy link
Contributor

@afchung afchung commented Jun 27, 2016

…Event

This addressed the issue by

  • Changing the Evaluator to fail when throwing an Exception in the Driver message handler and adding a test.

JIRA:
REEF-1431

@afchung
Copy link
Contributor Author

afchung commented Jun 27, 2016

This will conflict with #1042. I'll perform the merge when one of them gets merged into master. Thanks!

@jwang98052
Copy link
Contributor

I will have a look.

}
catch (Exception e)
{
Utilities.Diagnostics.Exceptions.Caught(e, Level.Error, "Error during message delivery.", Logger);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you want it ends up as FailedEvaluator instead of FailedTask, right? Can you document it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is documented in the JIRA.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll also add a comment here.

@jwang98052
Copy link
Contributor

I completed a pass.

@afchung
Copy link
Contributor Author

afchung commented Jun 28, 2016

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

@jwang98052
Copy link
Contributor

LGTM with a minor comment.

@afchung
Copy link
Contributor Author

afchung commented Jun 29, 2016

@jwang98052 I've replied to your comment. Thanks!

@afchung
Copy link
Contributor Author

afchung commented Jun 29, 2016

I've resolved the conflict and merged with latest master.

@tcNickolas
Copy link
Member

This PR shows conflicts again. Could you please resolve?

@afchung
Copy link
Contributor Author

afchung commented Jun 29, 2016

I've resolved the conflict and merged with latest master.

@tcNickolas
Copy link
Member

Conflicts again.
On the bright side, I think I've fixed the problem with Jenkins, so on next iteration it should pass.

@afchung
Copy link
Contributor Author

afchung commented Jun 30, 2016

Right...Git doesn't know how to resolve .csproj conflicts :(

…Event

This addressed the issue by
  * Changing the Evaluator to fail when throwing an Exception in the Driver message handler and adding a test.

JIRA:
  [REEF-1431](https://issues.apache.org/jira/browse/REEF-1431)
@afchung
Copy link
Contributor Author

afchung commented Jun 30, 2016

@tcNickolas I've resolved the conflict, please have another look. Thanks!

/// <summary>
/// An test EventHandle that simply wraps around a <see cref="ManualResetEventSlim"/>.
/// </summary>
public sealed class EventHandle
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a less generic name for this class? EventHandle has no information about what kind of event handling happens inside

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't handle an Event, it is a handle for an Event. I've renamed to EventMonitor for clarity.

@afchung
Copy link
Contributor Author

afchung commented Jun 30, 2016

@tcNickolas I've addressed your comment, please have another look. Thanks!

@tcNickolas
Copy link
Member

LGTM, will merge

@asfgit asfgit closed this in 641cb59 Jul 1, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants