[SPARK-18742][CORE]Clarify that user-defined BroadcastFactory is not supported

[windpiger]

What changes were proposed in this pull request?

After SPARK-12588 Remove HTTPBroadcast [1], the one and only implementation of BroadcastFactory is TorrentBroadcastFactory and the spark.broadcast.factory conf has removed.

however the scaladoc says [2]:

/**

• An interface for all the broadcast implementations in Spark (to allow
• multiple broadcast implementations). SparkContext uses a user-specified
• BroadcastFactory implementation to instantiate a particular broadcast for the
• entire Spark job.
  */

so we should modify the comment that SparkContext will not use a user-specified BroadcastFactory implementation

[1] https://issues.apache.org/jira/browse/SPARK-12588
[2] https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/broadcast/BroadcastFactory.scala#L25-L30

How was this patch tested?

unit test added

[SparkQA]

Test build #69732 has finished for PR 16173 at commit a17d325.

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

[SparkQA]

Test build #69733 has finished for PR 16173 at commit 1b4d4b6.

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

[SparkQA]

Test build #69734 has finished for PR 16173 at commit f23becc.

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

[srowen]

You certainly have a point there, but I also don't know that there's actually an intent to do the work to truly support and maintain an SPI for broadcasts.

[windpiger]

since the trait BroadcastFactory is design for multiple implements, is't reasonable to provide a way to change the implement of BroadcastFactory? or only TorrentBroadcastFactory is enough

[srowen]

It existed to support several internal implementations, of which there is only one now. I don't know that it was ever intended for external implementations.

[windpiger]

ok, the BroadcastFactory' comment shows SparkContext uses a user-specified BroadcastFactory implementation to instantiate a particular broadcast for the entire Spark job. so I think it is designed for external implementations.

[srowen]

Hm, that's a good point. I don't recall ever hearing much about someone actually plugging in an implementation. It's a modest change you're proposing and not unreasonable, I just don't know how much this API is actually robust right now, or whether it really should be.

[windpiger]

when HttpBroadCastFactory & TorrentBroadcastFactory both exists, it is use spark.broadcast.factory to change the implement.

it is better to modify the comment of BroadcastFactory than readd the spark.broadcast.factory back?

[windpiger]

@srowen

[srowen]

Yeah those were Spark classes. This property doesn't seem to be used now.
It's possible to restore this but I don't know if it's intended now. Yes I suppose you could update the comment instead, but it doesn't seem like a big deal.

[windpiger]

ok,someone else can tell if it is resonable to readd the conf now?

[windpiger]

cc @rxin could you help to give some advise? thanks!

[rxin]

Is there actually an implementation you want to do? I'd push back against opening this up unless we have a good reason to (e.g. more scalable broadcast).

[windpiger]

if the trait BroadcastFactory is designed for user not for internal, it is better to add this conf eventhough there is no a more scalable broadcast. Or we can modify the comment https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/broadcast/BroadcastFactory.scala#L25-L30

[srowen]

I wouldn't open it up just to open it up. If there were a pretty good use case for external implementations then sure. Otherwise, at best, change the comment, yeah.

[windpiger]

ok,just modify the comment, thanks!

[SparkQA]

Test build #70168 has finished for PR 16173 at commit 49d799f.

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

[SparkQA]

Test build #3499 has finished for PR 16173 at commit 49d799f.

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

[windpiger]

retest this please

[SparkQA]

Test build #70227 has finished for PR 16173 at commit 49d799f.

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

[srowen]

@windpiger you need to update the title/description of this and the JIRA too.

[windpiger]

ok,thanks