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-6602][Core] Update Master, Worker, Client, AppClient and related classes to use RpcEndpoint #5392

Closed
wants to merge 24 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@zsxwing
Member

zsxwing commented Apr 7, 2015

This PR updates the rest Actors in core to RpcEndpoint.

Because there is no ActorSelection in RpcEnv, I changes the logic of registerWithMaster in Worker and AppClient to avoid blocking the message loop. These changes need to be reviewed carefully.

@zsxwing

This comment has been minimized.

Show comment
Hide comment
@zsxwing
Member

zsxwing commented Apr 7, 2015

cc @rxin

@SparkQA

This comment has been minimized.

Show comment
Hide comment
@SparkQA

SparkQA Apr 7, 2015

Test build #29794 has finished for PR 5392 at commit b776817.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Heartbeat(workerId: String, worker: RpcEndpointRef) extends DeployMessage
    • case class RegisteredWorker(master: RpcEndpointRef, masterWebUiUrl: String) extends DeployMessage
    • case class RegisterApplication(appDescription: ApplicationDescription, driver: RpcEndpointRef)
    • case class RegisteredApplication(appId: String, master: RpcEndpointRef) extends DeployMessage
    • case class MasterChanged(master: RpcEndpointRef, masterWebUiUrl: String)
  • This patch does not change any dependencies.

SparkQA commented Apr 7, 2015

Test build #29794 has finished for PR 5392 at commit b776817.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Heartbeat(workerId: String, worker: RpcEndpointRef) extends DeployMessage
    • case class RegisteredWorker(master: RpcEndpointRef, masterWebUiUrl: String) extends DeployMessage
    • case class RegisterApplication(appDescription: ApplicationDescription, driver: RpcEndpointRef)
    • case class RegisteredApplication(appId: String, master: RpcEndpointRef) extends DeployMessage
    • case class MasterChanged(master: RpcEndpointRef, masterWebUiUrl: String)
  • This patch does not change any dependencies.
@rxin

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/client/AppClient.scala
@rxin

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala
@vanzin

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/Client.scala
@vanzin

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/Client.scala
@vanzin

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/Client.scala
@vanzin

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/Client.scala
@vanzin

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/Client.scala
@vanzin

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/Client.scala
@vanzin

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/client/AppClient.scala
case RegisteredApplication(appId_, masterUrl) =>
override def receive: PartialFunction[Any, Unit] = {
case RegisteredApplication(appId_, masterRef) =>
// FIXME How to handle the following cases?

This comment has been minimized.

@vanzin

vanzin Apr 7, 2015

Contributor

So, this is the kind of thing that sendWithReply was meant to do. Can that be used here instead?

That way, if the call fails, you know (with a good probability) the app was not registered. At that point I'm not sure what the protocol is; send the request to the next master? Give up?

But in any case, this is one case where the "ask" pattern is really weird.

@vanzin

vanzin Apr 7, 2015

Contributor

So, this is the kind of thing that sendWithReply was meant to do. Can that be used here instead?

That way, if the call fails, you know (with a good probability) the app was not registered. At that point I'm not sure what the protocol is; send the request to the next master? Give up?

But in any case, this is one case where the "ask" pattern is really weird.

This comment has been minimized.

@zsxwing

zsxwing Apr 8, 2015

Member

So, this is the kind of thing that sendWithReply was meant to do. Can that be used here instead?

Both RegisterApplication and RegisteredApplication are sent using send. It's not the ask pattern.

My comment here is about sending RegisterApplication multiple times in registerWithMaster.

  1. In extreme case, RegisterApplication may need REGISTRATION_TIMEOUT_SECONDS to arrive at Master. Then we will send more than one RegisterApplications to the Master. However, this looks the user's duty. They should increase REGISTRATION_TIMEOUT_SECONDS.
  2. Considering the following order
   a. AppClient sends `RegisterApplication` to Master A and standby Master B.
   b. Master A receives RegisterApplication, and sends `RegisteredApplication` back.
   c. Master A crashes.
   d. Master B starts to recovery.
   e. `RegisterApplication` arrives at Master B, and Master B sends `RegisteredApplication` back.
   f. `RegisterApplication`from Master B arrives at AppClient
   g. `RegisterApplication`from Master A arrives at AppClient

Because this rarely happens, it may be not a big deal.

@zsxwing

zsxwing Apr 8, 2015

Member

So, this is the kind of thing that sendWithReply was meant to do. Can that be used here instead?

Both RegisterApplication and RegisteredApplication are sent using send. It's not the ask pattern.

My comment here is about sending RegisterApplication multiple times in registerWithMaster.

  1. In extreme case, RegisterApplication may need REGISTRATION_TIMEOUT_SECONDS to arrive at Master. Then we will send more than one RegisterApplications to the Master. However, this looks the user's duty. They should increase REGISTRATION_TIMEOUT_SECONDS.
  2. Considering the following order
   a. AppClient sends `RegisterApplication` to Master A and standby Master B.
   b. Master A receives RegisterApplication, and sends `RegisteredApplication` back.
   c. Master A crashes.
   d. Master B starts to recovery.
   e. `RegisterApplication` arrives at Master B, and Master B sends `RegisteredApplication` back.
   f. `RegisterApplication`from Master B arrives at AppClient
   g. `RegisterApplication`from Master A arrives at AppClient

Because this rarely happens, it may be not a big deal.

This comment has been minimized.

@vanzin

vanzin Apr 8, 2015

Contributor

Both RegisterApplication and RegisteredApplication are sent using send. It's not the ask pattern.

Yes, but they expect a sort of reply (RegisteredApplication) that is handled in the receive method. So it's kinda like the ask pattern, in a weird way.

As I mentioned, I'm not 100% familiar with this protocol. It may be that I misunderstood what these message do. But since there seems to be a clear reply expected for this message, that feels a lot like a job for sendWithReply. Then half of the questions you ask in the comments you added don't apply.

@vanzin

vanzin Apr 8, 2015

Contributor

Both RegisterApplication and RegisteredApplication are sent using send. It's not the ask pattern.

Yes, but they expect a sort of reply (RegisteredApplication) that is handled in the receive method. So it's kinda like the ask pattern, in a weird way.

As I mentioned, I'm not 100% familiar with this protocol. It may be that I misunderstood what these message do. But since there seems to be a clear reply expected for this message, that feels a lot like a job for sendWithReply. Then half of the questions you ask in the comments you added don't apply.

@vanzin

This comment has been minimized.

Show comment
Hide comment
@vanzin

vanzin Apr 7, 2015

Contributor

So I took a quick look, but I'm not familiar enough with the original code to provide good, informed feedback. The original protocol seems really weird to me, and I guess that leaks into the new code too. But fixing the protocol is probably out of the scope of this change...

Contributor

vanzin commented Apr 7, 2015

So I took a quick look, but I'm not familiar enough with the original code to provide good, informed feedback. The original protocol seems really weird to me, and I guess that leaks into the new code too. But fixing the protocol is probably out of the scope of this change...

@zsxwing

This comment has been minimized.

Show comment
Hide comment
@zsxwing

zsxwing Apr 8, 2015

Member

I used the following codes to handle errors for the new codes in Client but kept the old codes untouched.

logError(e.getMessage, e)
System.exit(SparkExitCode.UNCAUGHT_EXCEPTION)
Member

zsxwing commented Apr 8, 2015

I used the following codes to handle errors for the new codes in Client but kept the old codes untouched.

logError(e.getMessage, e)
System.exit(SparkExitCode.UNCAUGHT_EXCEPTION)
@SparkQA

This comment has been minimized.

Show comment
Hide comment
@SparkQA

SparkQA Apr 8, 2015

Test build #29860 has finished for PR 5392 at commit 4d1633b.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Heartbeat(workerId: String, worker: RpcEndpointRef) extends DeployMessage
    • case class RegisteredWorker(master: RpcEndpointRef, masterWebUiUrl: String) extends DeployMessage
    • case class RegisterApplication(appDescription: ApplicationDescription, driver: RpcEndpointRef)
    • case class RegisteredApplication(appId: String, master: RpcEndpointRef) extends DeployMessage
    • case class MasterChanged(master: RpcEndpointRef, masterWebUiUrl: String)
  • This patch does not change any dependencies.

SparkQA commented Apr 8, 2015

Test build #29860 has finished for PR 5392 at commit 4d1633b.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Heartbeat(workerId: String, worker: RpcEndpointRef) extends DeployMessage
    • case class RegisteredWorker(master: RpcEndpointRef, masterWebUiUrl: String) extends DeployMessage
    • case class RegisterApplication(appDescription: ApplicationDescription, driver: RpcEndpointRef)
    • case class RegisteredApplication(appId: String, master: RpcEndpointRef) extends DeployMessage
    • case class MasterChanged(master: RpcEndpointRef, masterWebUiUrl: String)
  • This patch does not change any dependencies.
@SparkQA

This comment has been minimized.

Show comment
Hide comment
@SparkQA

SparkQA Apr 8, 2015

Test build #29861 has finished for PR 5392 at commit e8ad0a5.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Heartbeat(workerId: String, worker: RpcEndpointRef) extends DeployMessage
    • case class RegisteredWorker(master: RpcEndpointRef, masterWebUiUrl: String) extends DeployMessage
    • case class RegisterApplication(appDescription: ApplicationDescription, driver: RpcEndpointRef)
    • case class RegisteredApplication(appId: String, master: RpcEndpointRef) extends DeployMessage
    • case class MasterChanged(master: RpcEndpointRef, masterWebUiUrl: String)
  • This patch does not change any dependencies.

SparkQA commented Apr 8, 2015

Test build #29861 has finished for PR 5392 at commit e8ad0a5.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Heartbeat(workerId: String, worker: RpcEndpointRef) extends DeployMessage
    • case class RegisteredWorker(master: RpcEndpointRef, masterWebUiUrl: String) extends DeployMessage
    • case class RegisterApplication(appDescription: ApplicationDescription, driver: RpcEndpointRef)
    • case class RegisteredApplication(appId: String, master: RpcEndpointRef) extends DeployMessage
    • case class MasterChanged(master: RpcEndpointRef, masterWebUiUrl: String)
  • This patch does not change any dependencies.
@SparkQA

This comment has been minimized.

Show comment
Hide comment
@SparkQA

SparkQA Apr 8, 2015

Test build #29867 has finished for PR 5392 at commit 7fdee0e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Heartbeat(workerId: String, worker: RpcEndpointRef) extends DeployMessage
    • case class RegisteredWorker(master: RpcEndpointRef, masterWebUiUrl: String) extends DeployMessage
    • case class RegisterApplication(appDescription: ApplicationDescription, driver: RpcEndpointRef)
    • case class RegisteredApplication(appId: String, master: RpcEndpointRef) extends DeployMessage
    • case class MasterChanged(master: RpcEndpointRef, masterWebUiUrl: String)
  • This patch does not change any dependencies.

SparkQA commented Apr 8, 2015

Test build #29867 has finished for PR 5392 at commit 7fdee0e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Heartbeat(workerId: String, worker: RpcEndpointRef) extends DeployMessage
    • case class RegisteredWorker(master: RpcEndpointRef, masterWebUiUrl: String) extends DeployMessage
    • case class RegisterApplication(appDescription: ApplicationDescription, driver: RpcEndpointRef)
    • case class RegisteredApplication(appId: String, master: RpcEndpointRef) extends DeployMessage
    • case class MasterChanged(master: RpcEndpointRef, masterWebUiUrl: String)
  • This patch does not change any dependencies.
Merge remote-tracking branch 'origin/master' into rpc-rewrite-part3
Conflicts:
	core/src/main/scala/org/apache/spark/deploy/LocalSparkCluster.scala
	core/src/main/scala/org/apache/spark/deploy/client/TestClient.scala
	core/src/test/scala/org/apache/spark/deploy/rest/StandaloneRestSubmitSuite.scala
@SparkQA

This comment has been minimized.

Show comment
Hide comment
@SparkQA

SparkQA Apr 14, 2015

Test build #30230 has finished for PR 5392 at commit 6637e3c.

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

SparkQA commented Apr 14, 2015

Test build #30230 has finished for PR 5392 at commit 6637e3c.

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

This comment has been minimized.

Show comment
Hide comment
@zsxwing

zsxwing Apr 14, 2015

Member

@vanzin I agree that this PR should focus on following the previous protocol instead of fixing issues of the protocol. Do you have other comments?

Member

zsxwing commented Apr 14, 2015

@vanzin I agree that this PR should focus on following the previous protocol instead of fixing issues of the protocol. Do you have other comments?

@SparkQA

This comment has been minimized.

Show comment
Hide comment
@SparkQA

SparkQA Apr 14, 2015

Test build #30233 has finished for PR 5392 at commit fadbb9e.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Heartbeat(workerId: String, worker: RpcEndpointRef) extends DeployMessage
    • case class RegisteredWorker(master: RpcEndpointRef, masterWebUiUrl: String) extends DeployMessage
    • case class RegisterApplication(appDescription: ApplicationDescription, driver: RpcEndpointRef)
    • case class RegisteredApplication(appId: String, master: RpcEndpointRef) extends DeployMessage
    • case class MasterChanged(master: RpcEndpointRef, masterWebUiUrl: String)
  • This patch does not change any dependencies.

SparkQA commented Apr 14, 2015

Test build #30233 has finished for PR 5392 at commit fadbb9e.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Heartbeat(workerId: String, worker: RpcEndpointRef) extends DeployMessage
    • case class RegisteredWorker(master: RpcEndpointRef, masterWebUiUrl: String) extends DeployMessage
    • case class RegisterApplication(appDescription: ApplicationDescription, driver: RpcEndpointRef)
    • case class RegisteredApplication(appId: String, master: RpcEndpointRef) extends DeployMessage
    • case class MasterChanged(master: RpcEndpointRef, masterWebUiUrl: String)
  • This patch does not change any dependencies.
@zsxwing

This comment has been minimized.

Show comment
Hide comment
@zsxwing

zsxwing Apr 14, 2015

Member

retest this please.

Member

zsxwing commented Apr 14, 2015

retest this please.

@SparkQA

This comment has been minimized.

Show comment
Hide comment
@SparkQA

SparkQA Apr 14, 2015

Test build #30236 has finished for PR 5392 at commit fadbb9e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Heartbeat(workerId: String, worker: RpcEndpointRef) extends DeployMessage
    • case class RegisteredWorker(master: RpcEndpointRef, masterWebUiUrl: String) extends DeployMessage
    • case class RegisterApplication(appDescription: ApplicationDescription, driver: RpcEndpointRef)
    • case class RegisteredApplication(appId: String, master: RpcEndpointRef) extends DeployMessage
    • case class MasterChanged(master: RpcEndpointRef, masterWebUiUrl: String)
  • This patch does not change any dependencies.

SparkQA commented Apr 14, 2015

Test build #30236 has finished for PR 5392 at commit fadbb9e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Heartbeat(workerId: String, worker: RpcEndpointRef) extends DeployMessage
    • case class RegisteredWorker(master: RpcEndpointRef, masterWebUiUrl: String) extends DeployMessage
    • case class RegisterApplication(appDescription: ApplicationDescription, driver: RpcEndpointRef)
    • case class RegisteredApplication(appId: String, master: RpcEndpointRef) extends DeployMessage
    • case class MasterChanged(master: RpcEndpointRef, masterWebUiUrl: String)
  • This patch does not change any dependencies.
@vanzin

This comment has been minimized.

Show comment
Hide comment
@vanzin

vanzin Apr 14, 2015

Contributor

@zsxwing not really, I'm not familiar enough with the code to comment. Maybe @rxin or @andrewor14?

Contributor

vanzin commented Apr 14, 2015

@zsxwing not really, I'm not familiar enough with the code to comment. Maybe @rxin or @andrewor14?

private var master: Option[RpcEndpointRef] = None
// To avoid calling listener.disconnected() multiple times
private var alreadyDisconnected = false
@volatile private var alreadyDead = false // To avoid calling listener.dead() multiple times

This comment has been minimized.

@aarondav

aarondav Jun 29, 2015

Contributor

It's not generally true that ThreadSafeRpcEndpoints require their mutable state to be volatile, right? Perhaps this is just being modified from a separate thread pool?

@aarondav

aarondav Jun 29, 2015

Contributor

It's not generally true that ThreadSafeRpcEndpoints require their mutable state to be volatile, right? Perhaps this is just being modified from a separate thread pool?

This comment has been minimized.

@zsxwing

zsxwing Jun 30, 2015

Member

It may be updated in a separate thread pool or the message loop of ClientEndpoint, so it's volatile.

@zsxwing

zsxwing Jun 30, 2015

Member

It may be updated in a separate thread pool or the message loop of ClientEndpoint, so it's volatile.

@aarondav

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/client/AppClient.scala
@aarondav

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/client/AppClient.scala
@aarondav

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/client/AppClient.scala
@aarondav

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala
@aarondav

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala
@aarondav

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala
@aarondav

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala
@aarondav

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala
@aarondav

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala
@aarondav

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala
@aarondav

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala
@@ -466,23 +531,13 @@ private[worker] class Worker(
case _ =>
logDebug(s"Driver $driverId changed state to $state")
}
master ! DriverStateChanged(driverId, state, exception)
sendToMaster(driverStageChanged)

This comment has been minimized.

@aarondav

aarondav Jun 29, 2015

Contributor

This change was not strictly necessary, right? Maybe it used to have a self reference.

@aarondav

aarondav Jun 29, 2015

Contributor

This change was not strictly necessary, right? Maybe it used to have a self reference.

This comment has been minimized.

@zsxwing

zsxwing Jun 30, 2015

Member

Could you clarify this point? This is same as the previous codes.

@zsxwing

zsxwing Jun 30, 2015

Member

Could you clarify this point? This is same as the previous codes.

This comment has been minimized.

@aarondav

aarondav Jun 30, 2015

Contributor

Rather, the change about the driverStageChanged @ instead of recreating DriverStateChanged(driverId, state, exception). It's not particularly important, but I was just curious if there was a reason.

@aarondav

aarondav Jun 30, 2015

Contributor

Rather, the change about the driverStageChanged @ instead of recreating DriverStateChanged(driverId, state, exception). It's not particularly important, but I was just curious if there was a reason.

This comment has been minimized.

@zsxwing

zsxwing Jul 1, 2015

Member

Ah, it's just for avoiding to create a new instance.

@zsxwing

zsxwing Jul 1, 2015

Member

Ah, it's just for avoiding to create a new instance.

@aarondav

View changes

Show outdated Hide outdated core/src/main/scala/org/apache/spark/deploy/worker/Worker.scala
@aarondav

This comment has been minimized.

Show comment
Hide comment
@aarondav

aarondav Jun 29, 2015

Contributor

The core logic all looks good to me, just had some nits.

Contributor

aarondav commented Jun 29, 2015

The core logic all looks good to me, just had some nits.

@SparkQA

This comment has been minimized.

Show comment
Hide comment
@SparkQA

SparkQA Jun 30, 2015

Test build #36121 has finished for PR 5392 at commit 2de7bed.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Heartbeat(workerId: String, worker: RpcEndpointRef) extends DeployMessage
    • case class RegisteredWorker(master: RpcEndpointRef, masterWebUiUrl: String) extends DeployMessage
    • case class RegisterApplication(appDescription: ApplicationDescription, driver: RpcEndpointRef)
    • case class RegisteredApplication(appId: String, master: RpcEndpointRef) extends DeployMessage
    • case class SubmitDriverResponse(
    • case class KillDriverResponse(
    • case class MasterChanged(master: RpcEndpointRef, masterWebUiUrl: String)

SparkQA commented Jun 30, 2015

Test build #36121 has finished for PR 5392 at commit 2de7bed.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Heartbeat(workerId: String, worker: RpcEndpointRef) extends DeployMessage
    • case class RegisteredWorker(master: RpcEndpointRef, masterWebUiUrl: String) extends DeployMessage
    • case class RegisterApplication(appDescription: ApplicationDescription, driver: RpcEndpointRef)
    • case class RegisteredApplication(appId: String, master: RpcEndpointRef) extends DeployMessage
    • case class SubmitDriverResponse(
    • case class KillDriverResponse(
    • case class MasterChanged(master: RpcEndpointRef, masterWebUiUrl: String)
@zsxwing

This comment has been minimized.

Show comment
Hide comment
@zsxwing

zsxwing Jun 30, 2015

Member

retest this please

Member

zsxwing commented Jun 30, 2015

retest this please

@SparkQA

This comment has been minimized.

Show comment
Hide comment
@SparkQA

SparkQA Jun 30, 2015

Test build #36147 has finished for PR 5392 at commit 2de7bed.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Heartbeat(workerId: String, worker: RpcEndpointRef) extends DeployMessage
    • case class RegisteredWorker(master: RpcEndpointRef, masterWebUiUrl: String) extends DeployMessage
    • case class RegisterApplication(appDescription: ApplicationDescription, driver: RpcEndpointRef)
    • case class RegisteredApplication(appId: String, master: RpcEndpointRef) extends DeployMessage
    • case class SubmitDriverResponse(
    • case class KillDriverResponse(
    • case class MasterChanged(master: RpcEndpointRef, masterWebUiUrl: String)

SparkQA commented Jun 30, 2015

Test build #36147 has finished for PR 5392 at commit 2de7bed.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Heartbeat(workerId: String, worker: RpcEndpointRef) extends DeployMessage
    • case class RegisteredWorker(master: RpcEndpointRef, masterWebUiUrl: String) extends DeployMessage
    • case class RegisterApplication(appDescription: ApplicationDescription, driver: RpcEndpointRef)
    • case class RegisteredApplication(appId: String, master: RpcEndpointRef) extends DeployMessage
    • case class SubmitDriverResponse(
    • case class KillDriverResponse(
    • case class MasterChanged(master: RpcEndpointRef, masterWebUiUrl: String)
@zsxwing

This comment has been minimized.

Show comment
Hide comment
@zsxwing

zsxwing Jun 30, 2015

Member

retest this please

Member

zsxwing commented Jun 30, 2015

retest this please

@SparkQA

This comment has been minimized.

Show comment
Hide comment
@SparkQA

SparkQA Jun 30, 2015

Test build #36148 has finished for PR 5392 at commit 2de7bed.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Heartbeat(workerId: String, worker: RpcEndpointRef) extends DeployMessage
    • case class RegisteredWorker(master: RpcEndpointRef, masterWebUiUrl: String) extends DeployMessage
    • case class RegisterApplication(appDescription: ApplicationDescription, driver: RpcEndpointRef)
    • case class RegisteredApplication(appId: String, master: RpcEndpointRef) extends DeployMessage
    • case class SubmitDriverResponse(
    • case class KillDriverResponse(
    • case class MasterChanged(master: RpcEndpointRef, masterWebUiUrl: String)

SparkQA commented Jun 30, 2015

Test build #36148 has finished for PR 5392 at commit 2de7bed.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Heartbeat(workerId: String, worker: RpcEndpointRef) extends DeployMessage
    • case class RegisteredWorker(master: RpcEndpointRef, masterWebUiUrl: String) extends DeployMessage
    • case class RegisterApplication(appDescription: ApplicationDescription, driver: RpcEndpointRef)
    • case class RegisteredApplication(appId: String, master: RpcEndpointRef) extends DeployMessage
    • case class SubmitDriverResponse(
    • case class KillDriverResponse(
    • case class MasterChanged(master: RpcEndpointRef, masterWebUiUrl: String)
@@ -504,6 +518,7 @@ private[master] class Master(
}
private def completeRecovery() {
// TODO Why synchronized

This comment has been minimized.

@aarondav

aarondav Jun 30, 2015

Contributor

This was due to an earlier state in the code when this method could be invoked from a different thread. It can be safely removed now.

@aarondav

aarondav Jun 30, 2015

Contributor

This was due to an earlier state in the code when this method could be invoked from a different thread. It can be safely removed now.

@aarondav

This comment has been minimized.

Show comment
Hide comment
@aarondav

aarondav Jun 30, 2015

Contributor

Just one comment thanks to your proactive TODOifying :) LGTM, feel free to merge after.

Contributor

aarondav commented Jun 30, 2015

Just one comment thanks to your proactive TODOifying :) LGTM, feel free to merge after.

@rxin

This comment has been minimized.

Show comment
Hide comment
@rxin

rxin Jul 1, 2015

Contributor

Alright I'm going to merge this. @zsxwing please submit a separate PR to address the TODO.

Contributor

rxin commented Jul 1, 2015

Alright I'm going to merge this. @zsxwing please submit a separate PR to address the TODO.

@asfgit asfgit closed this in 3bee0f1 Jul 1, 2015

asfgit pushed a commit that referenced this pull request Jul 1, 2015

[SPARK-6602][Core]Remove unnecessary synchronized
A follow-up pr to address #5392 (comment)

Author: zsxwing <zsxwing@gmail.com>

Closes #7141 from zsxwing/pr5392-follow-up and squashes the following commits:

fcf7b50 [zsxwing] Remove unnecessary synchronized

@zsxwing zsxwing deleted the zsxwing:rpc-rewrite-part3 branch Dec 23, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment