Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-6028][Core]Remerge #6457: new RPC implemetation and also pick #8905 #8944

Closed
wants to merge 6 commits into from
Closed

Conversation

zsxwing
Copy link
Member

@zsxwing zsxwing commented Sep 30, 2015

This PR just reverted 02144d6 to remerge #6457 and also included the commits in #8905.

@zsxwing
Copy link
Member Author

zsxwing commented Sep 30, 2015

/cc @rxin @vanzin

@SparkQA
Copy link

SparkQA commented Sep 30, 2015

Test build #43123 has finished for PR 8944 at commit ffe4b97.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@zsxwing
Copy link
Member Author

zsxwing commented Sep 30, 2015

Looks like a red flag:

Test Result (1 failure / +1)
org.apache.spark.rpc.netty.NettyRpcEnvSuite.self: call in onStop

I will investigate it.

@zsxwing
Copy link
Member Author

zsxwing commented Oct 2, 2015

retest this please

@SparkQA
Copy link

SparkQA commented Oct 2, 2015

Test build #43177 has finished for PR 8944 at commit ffe4b97.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 2, 2015

Test build #43178 has finished for PR 8944 at commit 534f599.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

…ncurrent` to `true`

`enableConcurrent = true` is in a closure so we should not use `synchronized` directly because that will use the closure itself as the monitor object. The correct monitor object should be the Inbox instance.
@SparkQA
Copy link

SparkQA commented Oct 2, 2015

Test build #43180 has started for PR 8944 at commit 77f08f6.

… is stopped

Because `Inbox.stop()` may run before processing `OnStart`, it's possible that `enableConcurrent` will be set to `true` after Inbox is stopped in the previous codes. This is not a correct behavior.

This commit just added a check before setting `enableConcurrent` to `true` to fix this issue.
@SparkQA
Copy link

SparkQA commented Oct 2, 2015

Test build #43182 has started for PR 8944 at commit a574d70.

@vanzin
Copy link
Contributor

vanzin commented Oct 2, 2015

I looked just at the recent patches, assuming the others just contain the code that had already been reviewed. LGTM pending tests.

@zsxwing
Copy link
Member Author

zsxwing commented Oct 3, 2015

retest this please

@SparkQA
Copy link

SparkQA commented Oct 3, 2015

Test build #43206 has finished for PR 8944 at commit a574d70.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@rxin
Copy link
Contributor

rxin commented Oct 3, 2015

I've merged this.

@asfgit asfgit closed this in 107320c Oct 3, 2015
@zsxwing zsxwing deleted the SPARK-6028 branch October 7, 2015 15:49
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