[SPARK-3709] Executors don't always report broadcast block removal properly back to the driver

[rxin]

The problem was that the 2nd argument in RemoveBroadcast is not tellMaster! It is "removeFromDriver". Basically when removeFromDriver is not true, we don't report broadcast block removal back to the driver, and then other executors mistakenly think that the executor would still have the block, and try to fetch from it.

cc @tdas

[SparkQA]

QA tests have started for PR 2588 at commit 2a13f70.

• This patch merges cleanly.

[SparkQA]

QA tests have started for PR 2588 at commit 2a13f70.

• This patch merges cleanly.

[aarondav]

LGTM. Merging into master, branch-1.1, and branch-1.0. Should I also backport to branch-0.9?

[rxin]

Make sure you backport in 0.3

[pwendell]

@aarondav I already merged this actually...

[rxin]

The problem was that the 2nd argument in RemoveBroadcast is not tellMaster! It is "removeFromDriver". Basically when removeFromDriver is not true, we don't report broadcast block removal back to the driver, and then other executors mistakenly think that the executor would still have the block, and try to fetch from it.

[SparkQA]

QA tests have started for PR 2588 at commit f430686.

• This patch merges cleanly.

[SparkQA]

QA tests have started for PR 2588 at commit f430686.

• This patch merges cleanly.

[SparkQA]

Tests timed out after a configured wait of 120m.

[AmplabJenkins]

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

[rxin]

@nchammas - would you be interested in submitting a pr to change the qa script so that the timeout and failure message already prints the commit hash?

[SparkQA]

Tests timed out after a configured wait of 120m.

[SparkQA]

QA tests have started for PR 2588 at commit 6dab2e3.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 2588 at commit f430686.

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

[SparkQA]

QA tests have finished for PR 2588 at commit f430686.

• This patch passes unit 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/21019/

[tdas]

Isnt there a way to augment the existing tests to make sure that the state in the driver (blockmanagermaster) is cleared after removing tests?

[rxin]

Yes - can you submit one? I'm going to merge this because it has been blocking a lot of other patches.

[tdas]

Actually, I took a look, it does test that. So I am not sure how it was passing earlier some of the times.

[rxin]

It worked because askSlaves was true and the driver always queries the slaves in your afterUnpersist test. The problem is with regard to reporting, not whether the block itself has been dropped or not.

[SparkQA]

QA tests have finished for PR 2588 at commit 6dab2e3.

• 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/21022/

[nchammas]

@nchammas - would you be interested in submitting a pr to change the qa script so that the timeout and failure message already prints the commit hash?

@rxin Straight failures should already include the commit hash, like here. (Note that messages like this one do not come from our script.)

I can make a PR to add the commit hash to the timeout messages.