[SPARK-4064]NioBlockTransferService.fetchBlocks may cause spark to hang.

[witgo]

cc @rxin

[SparkQA]

Test build #22137 has started for PR 2929 at commit f8f3195.

• This patch merges cleanly.

[SparkQA]

Test build #22137 has finished for PR 2929 at commit f8f3195.

• 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/22137/
Test PASSed.

[rxin]

this change looks good. but can you comment on when this would happen and put it into the source code?

thanks.

[rxin]

cc @aarondav since this will conflict with your change

[SparkQA]

Test build #22216 has started for PR 2929 at commit c23e562.

• This patch merges cleanly.

[SparkQA]

Test build #22216 has finished for PR 2929 at commit c23e562.

• 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/22216/
Test PASSed.

[pwendell]

Can this say "Received empty message from $cmId" or "Received no data from $cmId"?

[SparkQA]

Test build #22269 has started for PR 2929 at commit 20110f2.

• This patch merges cleanly.

[SparkQA]

Test build #22269 has finished for PR 2929 at commit 20110f2.

• 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/22269/
Test PASSed.

[rxin]

Thanks. I'm merging this one.

[rxin]

@aarondav this will conflict with your pr.

[JoshRosen]

This is moot now, since the related code has already changed, but I just wanted to point out that this PR seems subtly incorrect in cases where some but not all of of the blocks are missing in the remote block manager. As of this commit, getBlock returns null when blocks are missing, which ends up getting wrapped into a None in processBlock, so a fetch failure for a particular block results in the absence of a block result rather than an explicit error response. In some later commits, this was changed (perhaps unintentionally) so that fetch failures result in exceptions that result in connection manager error messages, so these errors are now handled in the future's onFailure handler.

In retrospect, I think that the right fix here would have been to either a) send negative error responses, or b) have the requester match the responses it received against the ids in its request and treat any missing blocks as failures.