Skip to content

Conversation

@zsxwing
Copy link
Member

@zsxwing zsxwing commented Apr 5, 2015

This is the second PR for [SPARK-6602]. It updated MapOutputTrackerMasterActor and its unit tests.

cc @rxin

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29730/
Test PASSed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does Akka always serialize exceptions? There are exceptions that cannot be serialized, and we should be careful there.

Can you add a unit test calling sendFailure with an unserializable exception to make sure things still work?

Copy link
Contributor

Choose a reason for hiding this comment

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

This also brings another question: what's the semantics in the case of exceptions? Are they ignored, logged and ignored, or propagated back to the caller?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are exceptions that cannot be serialized, and we should be careful there.

Because Throwable implements java.io.Serializable, I assume all exceptions can be serialized. Do you mean that some exception may throw an exception in writeObject(java.io.ObjectOutputStream out) to prevent it from being serialized?

Copy link
Contributor

Choose a reason for hiding this comment

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

In an ideal world all exceptions should be serializable, but I've seen in the past an exception that was not serializable (because it contained some field that was not serializable).

Copy link
Member Author

Choose a reason for hiding this comment

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

Such exception will be swallowed by Akka by default. I added ErrorMonitor to handle errors caught by Akka. Although they can not be serialized, at least we can log them.

Copy link
Contributor

Choose a reason for hiding this comment

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

One more thing - can you update the RpcEnv doc to specify what happens in the case of uncaught exceptions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@SparkQA
Copy link

SparkQA commented Apr 6, 2015

Test build #29736 has started for PR 5371 at commit 93c6c20.

@rxin
Copy link
Contributor

rxin commented Apr 6, 2015

LGTM.

@SparkQA
Copy link

SparkQA commented Apr 6, 2015

Test build #29737 has started for PR 5371 at commit 4013a22.

@SparkQA
Copy link

SparkQA commented Apr 6, 2015

Test build #29737 has finished for PR 5371 at commit 4013a22.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@AmplabJenkins
Copy link

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29737/
Test FAILed.

@SparkQA
Copy link

SparkQA commented Apr 6, 2015

Test build #29738 has started for PR 5371 at commit fcf3816.

@SparkQA
Copy link

SparkQA commented Apr 6, 2015

Test build #29736 has finished for PR 5371 at commit 93c6c20.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29736/
Test PASSed.

@SparkQA
Copy link

SparkQA commented Apr 6, 2015

Test build #29738 has finished for PR 5371 at commit fcf3816.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29738/
Test PASSed.

@rxin
Copy link
Contributor

rxin commented Apr 6, 2015

Merging in master. Thanks.

@asfgit asfgit closed this in 0b5d028 Apr 6, 2015
@zsxwing zsxwing deleted the rpc-rewrite-part2 branch April 6, 2015 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants