[SPARK-3741] Make ConnectionManager propagate errors properly and add mo...

[zsxwing]

...re logs to avoid Executors swallowing errors

This PR made the following changes:

• Register a callback to Connection so that the error will be propagated properly.
• Add more logs so that the errors won't be swallowed by Executors.
• Use trySuccess/tryFailure because Promise doesn't allow to call success/failure more than once.

[SparkQA]

QA tests have started for PR 2593 at commit 764aec5.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2593 at commit 764aec5.

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

[AmplabJenkins]

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

[zsxwing]

retest this please. This test is OK in my machine.

[SparkQA]

QA tests have started for PR 2593 at commit 764aec5.

• This patch merges cleanly.

[AmplabJenkins]

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

[JoshRosen]

Could you change this to be more descriptive? How about something like "Ignored error in onExceptionCallback"?

[JoshRosen]

Hi @zsxwing,

Thanks for submitting this PR (and sorry for the delayed review)! These changes will be very helpful in debugging certain types of connection manager issues that we've encountered. I like the careful attention to error-handling cases that we missed before, especially the use of afterExecute to detect unhandled errors.

I left a few comments, mostly regarding naming. If you fix up the merge conflicts, I think this looks ready to merge. Thanks!

[zsxwing]

I added NonFatal(t) to avoid to output fatal exceptions. It's expected that they are not be handled.

[JoshRosen]

Nice change.

[zsxwing]

Merged and updated the naming.

[SparkQA]

QA tests have started for PR 2593 at commit 1d5aed5.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2593 at commit 1d5aed5.

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

[AmplabJenkins]

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

[JoshRosen]

This looks good to me, so I'm going to merge it. Thanks a bunch; this will really help with debugging!