[SPARK-10827] [CORE] AppClient should not use `askWithReply` in `receiveAndReply` #9317

BryanCutler · 2015-10-28T05:14:54Z

Changed AppClient to be non-blocking in receiveAndReply by using a separate thread to wait for response and reply to the context. The threads are managed by a thread pool. Also added unit tests for the AppClient interface.

… a thread pool

…AndReply-SPARK-10827

…hecking

BryanCutler · 2015-10-28T05:18:33Z

@zsxwing would you mind checking this out? I noticed there were no existing unit tests for AppClient, so I added them. I couldn't think of a way to test that the calls are non-blocking, any ideas?

BryanCutler · 2015-10-28T05:34:40Z

core/src/test/scala/org/apache/spark/deploy/client/AppClientSuite.scala

Is there a way to tell if the client fails to connect to the master? It does log an error, but if I wanted to check to see if the AppClient endpoint was registered after calling start(), there doesn't seem to be a way.

So the private volatile var registered lets us check if we suggested at registering with a master is that what you were looking for?

Well, I was trying to think of making a test for AppClient in the case of an unreachable Master. The rpc env logs an exception right away, but the only way to tell from outside the AppClient is to set a listener with a connected callback, then poll to see if it ever gets hit. Maybe this isn't really an issue in practice though.

the only way to tell from outside the AppClient is to set a listener with a connected callback, then poll to see if it ever gets hit. Maybe this isn't really an issue in practice though.

I think that's fine.

SparkQA · 2015-10-28T06:57:00Z

Test build #44495 has finished for PR 9317 at commit 5e155cc.

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

BryanCutler · 2015-10-29T21:15:58Z

retest this please

SparkQA · 2015-10-29T23:00:15Z

Test build #44639 has finished for PR 9317 at commit 5e155cc.

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

BryanCutler · 2015-10-30T16:38:54Z

@holdenk if you wouldn't mind taking a look at this, I'd appreciate it!

holdenk · 2015-10-30T17:39:19Z

Sure thing :)

holdenk · 2015-10-30T17:46:57Z

core/src/main/scala/org/apache/spark/deploy/client/AppClient.scala

this code (the new runnable sending reply) seems to be duplicated a few times, maybe factor it out into a helper function?

At first I was trying to put these 2 calls into something like Utils.tryWithSafeFinally with blocks as arguments but the call ended up looking a little confusing as to what was happening.

maybe just a regular function like this would be better, although a little less flexible

private def receiveAndReplyAsync[T](masterRef: RpcEndpointRef, context: RpcCallContext, msg: T): Unit = { // execute ask and reply in thread pool ..

BryanCutler · 2015-10-30T19:24:53Z

Thanks for the feedback @holdenk !

… reduce code

BryanCutler · 2015-10-30T21:11:46Z

retest this please

SparkQA · 2015-10-30T22:45:50Z

Test build #44705 has finished for PR 9317 at commit 5e155cc.

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

…AndReply-SPARK-10827

BryanCutler · 2015-10-31T00:12:02Z

retest this please

BryanCutler · 2015-10-31T00:36:24Z

bad luck today I guess :(
jenkins retest this please

BryanCutler · 2015-10-31T01:09:04Z

retest this please

SparkQA · 2015-10-31T02:45:15Z

Test build #44722 has finished for PR 9317 at commit ab3e929.

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

…AndReply-SPARK-10827

BryanCutler · 2015-11-02T19:10:10Z

retest this please

SparkQA · 2015-11-02T19:25:16Z

Test build #44824 has finished for PR 9317 at commit 95d2499.

This patch fails to build.
This patch merges cleanly.
This patch adds the following public classes (experimental):\n * case class RegisteredExecutor(hostname: String) extends CoarseGrainedClusterMessage\n

BryanCutler · 2015-11-03T17:52:25Z

retest this please

SparkQA · 2015-11-03T19:40:05Z

Test build #44939 has finished for PR 9317 at commit 95d2499.

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

BryanCutler · 2015-11-03T20:01:42Z

Hi @zsxwing , could you take a look at this? There have been some superficial test failures, but the last one in CoarseMesosSchedulerBackendSuite looks like it could be real, but as far as I can tell, it does not use the AppClient.

cc @rxin @vanzin

zsxwing · 2015-11-03T20:15:50Z

core/src/main/scala/org/apache/spark/deploy/client/AppClient.scala

You can use org.apache.spark.util.ThreadUtils.newDaemonCachedThreadPool instead.

zsxwing · 2015-11-03T20:36:29Z

@BryanCutler Could you also make the following variables volatile? They are accessed in multiple threads.

  private var endpoint: RpcEndpointRef = null
  private var appId: String = null

zsxwing · 2015-11-03T21:06:14Z

core/src/main/scala/org/apache/spark/deploy/client/AppClient.scala

nit: indention. The correct indention is:

private def receiveAndReplyAsync[T]( masterRef: RpcEndpointRef, context: RpcCallContext, msg: T): Unit = {

BryanCutler · 2015-11-04T20:18:47Z

Thanks for the feedback @zsxwing !

@BryanCutler Could you also make the following variables volatile? They are accessed in multiple threads.

private var endpoint: RpcEndpointRef = null
private var appId: String = null

I made this change, but would you mind clarifying a little where these are shared? From what I can tell, I think the RpcEndpointRef gets wrapped in the NettyRpcCallContext but appId is just copied to the RequestExecutors and KillExecutors messages.

zsxwing · 2015-11-04T21:22:31Z

I made this change, but would you mind clarifying a little where these are shared? From what I can tell, I think the RpcEndpointRef gets wrapped in the NettyRpcCallContext but appId is just copied to the RequestExecutors and KillExecutors messages.

E.g., requestTotalExecutors is called from SparkDeploySchedulerBackend and it's not in the thread that setting endpoint and appId.

zsxwing · 2015-11-04T21:26:14Z

core/src/main/scala/org/apache/spark/deploy/client/AppClient.scala

nit: must be notified.

SparkQA · 2015-11-04T21:54:42Z

Test build #45044 has finished for PR 9317 at commit cd5329c.

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

BryanCutler · 2015-11-04T22:12:02Z

E.g., requestTotalExecutors is called from SparkDeploySchedulerBackend and it's not in the thread that setting endpoint and appId.

Got it, thanks!

SparkQA · 2015-11-04T23:54:21Z

Test build #45048 has finished for PR 9317 at commit 4f46cfd.

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

zsxwing · 2015-11-10T19:29:28Z

LGTM.

CC @rxin to take a final look

rxin · 2015-11-11T00:30:53Z

core/src/main/scala/org/apache/spark/deploy/client/AppClient.scala

it's better to use an atomic reference to be more explicit here. i'm going to merge this and update it.

…veAndReply` Changed AppClient to be non-blocking in `receiveAndReply` by using a separate thread to wait for response and reply to the context. The threads are managed by a thread pool. Also added unit tests for the AppClient interface. Author: Bryan Cutler <[email protected]> Closes #9317 from BryanCutler/appClient-receiveAndReply-SPARK-10827. (cherry picked from commit a398905) Signed-off-by: Reynold Xin <[email protected]>

This is a followup for #9317 to replace volatile fields with AtomicBoolean and AtomicReference. Author: Reynold Xin <[email protected]> Closes #9611 from rxin/SPARK-10827. (cherry picked from commit e1bcf6a) Signed-off-by: Reynold Xin <[email protected]>

This is a followup for #9317 to replace volatile fields with AtomicBoolean and AtomicReference. Author: Reynold Xin <[email protected]> Closes #9611 from rxin/SPARK-10827.

This is a followup for apache#9317 to replace volatile fields with AtomicBoolean and AtomicReference. Author: Reynold Xin <[email protected]> Closes apache#9611 from rxin/SPARK-10827.

BryanCutler added 4 commits October 23, 2015 10:34

[SPARK-10827] Changed AppClient receiveAndReply to non-blocking using…

3872cc6

… a thread pool

[SPARK-10827] Added unit tests for AppClient

80794e8

Merge remote-tracking branch 'upstream/master' into appClient-receive…

e33c2b4

…AndReply-SPARK-10827

[SPARK-10827] Used variable to clarify what result the test case is c…

5e155cc

…hecking

BryanCutler reviewed Oct 28, 2015
View reviewed changes

holdenk reviewed Oct 30, 2015
View reviewed changes

[SPARK-10827] Moved receive and reply execution to helper function to…

67bfd2a

… reduce code

Merge remote-tracking branch 'upstream/master' into appClient-receive…

ab3e929

…AndReply-SPARK-10827

Merge remote-tracking branch 'upstream/master' into appClient-receive…

95d2499

…AndReply-SPARK-10827

zsxwing reviewed Nov 3, 2015
View reviewed changes

BryanCutler added 2 commits November 3, 2015 14:25

Fixed indentation, imports, and spacing

3ae1151

Fixed threading issues, cleanup, added comments

cd5329c

zsxwing reviewed Nov 4, 2015
View reviewed changes

core/src/main/scala/org/apache/spark/deploy/client/AppClient.scala Outdated

Copy link

Member

zsxwing Nov 4, 2015

Choose a reason for hiding this comment

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

nit: must be notified.

Corrected grammar typo

4f46cfd

rxin reviewed Nov 11, 2015
View reviewed changes

asfgit closed this in a398905 Nov 11, 2015

rxin mentioned this pull request Nov 11, 2015

[SPARK-10827] replace volatile with Atomic* in AppClient.scala. #9611

Closed

BryanCutler deleted the appClient-receiveAndReply-SPARK-10827 branch November 18, 2015 21:37

[SPARK-10827] [CORE] AppClient should not use askWithReply in receiveAndReply #9317

[SPARK-10827] [CORE] AppClient should not use askWithReply in receiveAndReply #9317

Uh oh!

Conversation

BryanCutler commented Oct 28, 2015

Uh oh!

BryanCutler commented Oct 28, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 28, 2015

Uh oh!

BryanCutler commented Oct 29, 2015

Uh oh!

SparkQA commented Oct 29, 2015

Uh oh!

BryanCutler commented Oct 30, 2015

Uh oh!

holdenk commented Oct 30, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanCutler commented Oct 30, 2015

Uh oh!

BryanCutler commented Oct 30, 2015

Uh oh!

SparkQA commented Oct 30, 2015

Uh oh!

BryanCutler commented Oct 31, 2015

Uh oh!

BryanCutler commented Oct 31, 2015

Uh oh!

BryanCutler commented Oct 31, 2015

Uh oh!

SparkQA commented Oct 31, 2015

Uh oh!

BryanCutler commented Nov 2, 2015

Uh oh!

SparkQA commented Nov 2, 2015

Uh oh!

BryanCutler commented Nov 3, 2015

Uh oh!

SparkQA commented Nov 3, 2015

Uh oh!

BryanCutler commented Nov 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zsxwing commented Nov 3, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BryanCutler commented Nov 4, 2015

Uh oh!

zsxwing commented Nov 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 4, 2015

Uh oh!

BryanCutler commented Nov 4, 2015

Uh oh!

SparkQA commented Nov 4, 2015

Uh oh!

zsxwing commented Nov 10, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

[SPARK-10827] [CORE] AppClient should not use `askWithReply` in `receiveAndReply` #9317

[SPARK-10827] [CORE] AppClient should not use `askWithReply` in `receiveAndReply` #9317