[SPARK-12837][SPARK-20666][CORE][FOLLOWUP] getting name should not fail if accumulator is garbage collected

[cloud-fan]

What changes were proposed in this pull request?

After #17596 , we do not send internal accumulator name to executor side anymore, and always look up the accumulator name in AccumulatorContext.

This cause a regression if the accumulator is already garbage collected, this PR fixes this by still sending accumulator name for SQLMetrics.

How was this patch tested?

N/A

[cloud-fan]

cc @vanzin

[rxin]

What's the issue with SQL metrics?

[SparkQA]

Test build #76726 has started for PR 17931 at commit 5ae4f6e.

[cloud-fan]

we will keep SQLMetrics that sent from executors as UI data, while the actual accumulator registered for SQLMetrics may be garbage collected as the SparkPlan linked with it is garbage collected.

task context accumulators don't have this problem as we always keep the registered accumulators in DAGScheduler

[cloud-fan]

retest this please

[SparkQA]

Test build #76741 has finished for PR 17931 at commit 5ae4f6e.

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

[vanzin]

by still sending accumulator name for SQLMetrics

Do you have an idea of how much this undoes the benefits of SPARK-12837? You're still avoiding sending the names of internal metrics, but I don't have a feel for how many accumulators a large sql query might generate.

Also, is that really necessary? The errors seen in the bug (and that I noticed in my testing) were always on the driver side.

[cloud-fan]

at the time we received SQLMetrics in SQLListener with task end event, the registered accumulator may already be GCed, then there is no way to retrieve the accumulator names, except we sending accumulator names to executor side, so that when executor can send back accumulators to driver side with names.

[vanzin]

I see. What about the gains from SPARK-12837? Are they still enough that the change is justified, or should we just revert it instead?

[vanzin]

(BTW, if keeping the code, a slightly more verbose comment in the code explaining why non-internal accumulators still need to send their names would be good.)

[cloud-fan]

I checked the SPARK-12837 again and we only lose a little from this PR. I'll add more comments.

[SparkQA]

Test build #76937 has finished for PR 17931 at commit 2a3f773.

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

[vanzin]

LGTM.

[vanzin]

Merging to master / 2.2.