[SPARK-14699][Core]Stop endpoints before closing the connections and don't stop client in Outbox

[zsxwing]

What changes were proposed in this pull request?

In general, onDisconnected is for dealing with unexpected network disconnections. When RpcEnv.shutdown is called, the disconnections are expected so RpcEnv should not fire these events.

This PR moves dispatcher.stop() above closing the connections so that when stopping RpcEnv, the endpoints won't receive onDisconnected events.

In addition, Outbox should not close the client since it will be reused by others. This PR fixes it as well.

How was this patch tested?

test("SPARK-14699: RpcEnv.shutdown should not fire onDisconnected events")

[BryanCutler]

LGTM. I noticed this problem in testing out the DriverRunner for another issue and was looking for a fix. After applying this patch, the Driver will complete with a FINISHED status instead of FAILED (tested on a local cluster).

I did notice some new warnings in the Driver log though, but doesn't seem to cause a problem

WARN Dispatcher: Message RemoteProcessDisconnected(oc7258617548.ibm.com:7077) dropped. RpcEnv already stopped.

And the app log still has an error that the worker watcher was disconnect, but maybe that is a separate issue and it doesn't seem to affect the application final state

16/04/18 14:55:57 INFO CoarseGrainedExecutorBackend: Driver commanded a shutdown
16/04/18 14:55:57 INFO MemoryStore: MemoryStore cleared
16/04/18 14:55:57 INFO BlockManager: BlockManager stopped
16/04/18 14:55:57 ERROR WorkerWatcher: Lost connection to worker rpc endpoint spark://Worker@192.168.1.22:39166. Exiting.

[zsxwing]

WARN Dispatcher: Message RemoteProcessDisconnected(oc7258617548.ibm.com:7077) dropped. RpcEnv already stopped.

Yep, Netty may emit the network events after RpcEnv stops. But it doesn't matter since there are only a few warning logs.

16/04/18 14:55:57 ERROR WorkerWatcher: Lost connection to worker rpc endpoint spark://Worker@192.168.1.22:39166. Exiting.

Was the worker stopped at the same time? If the connection does disconnect before dispatcher.stop, this could happen.

[SparkQA]

Test build #56141 has finished for PR 12481 at commit 91c5bfd.

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

[BryanCutler]

Was the worker stopped at the same time?

No the worker wasn't stopped while running this. I think this error could be from this: when the application completes, the Master removes the application and kill executors, but if the rpcEnv in the CoarseGrainedExecutorBackend hasn't fully terminated then the WorkerWatcher will receive the onDisconnect message.

[zsxwing]

cc @andrewor14

[zsxwing]

No the worker wasn't stopped while running this. I think this error could be from this: when the application completes, the Master removes the application and kill executors, but if the rpcEnv in the CoarseGrainedExecutorBackend hasn't fully terminated then the WorkerWatcher will receive the onDisconnect message.

Looking the related codes.

[zsxwing]

@BryanCutler Fixed it in this PR. Outbox should not close the client since it will be reused by others.

[zsxwing]

cc @vanzin

[vanzin]

Minor, but using mockito (mock then verify) here would be a lot less code.

[zsxwing]

Updated

[vanzin]

Looks alright; I assume no existing code relies on the "onDisconnected" event for cleaning things up?

[zsxwing]

Looks alright; I assume no existing code relies on the "onDisconnected" event for cleaning things up?

No. Just went through all onDisconnected methods. Cleaning up should be done in onStop.

[SparkQA]

Test build #56395 has finished for PR 12481 at commit 7c78927.

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

[SparkQA]

Test build #56399 has finished for PR 12481 at commit cee0e42.

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

[zsxwing]

retest this please

[SparkQA]

Test build #56417 has finished for PR 12481 at commit cee0e42.

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

[zsxwing]

retest this please

[vanzin]

@zsxwing that test is failing everywhere, you don't need to bother retesting because of it.

[SparkQA]

Test build #56445 has finished for PR 12481 at commit cee0e42.

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

[zsxwing]

retest this please

[SparkQA]

Test build #56491 has finished for PR 12481 at commit cee0e42.

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

[BryanCutler]

@BryanCutler Fixed it in this PR. Outbox should not close the client since it will be reused by others.

I confirmed this fix takes care of the error in the app log

[zsxwing]

Thanks, merging to master.