[SPARK-14358] Change SparkListener from a trait to an abstract class #12142
Conversation
|
cc @JoshRosen |
|
Test build #54821 has finished for PR 12142 at commit
|
|
Test build #54823 has finished for PR 12142 at commit
|
| @@ -249,6 +250,8 @@ trait SparkListener { | |||
| * Called when other events like SQL-specific events are posted. | |||
| */ | |||
| def onOtherEvent(event: SparkListenerEvent) { } | |||
|
|
|||
| // WHENEVER WE ADD A METHOD HERE, PLEASE ALSO UPDATE SparkFirehoseListener. | |||
There was a problem hiding this comment.
We can have both this abstract class and also an interface? That way the SparkFirehoseListener can just implement the interface instead of extending the abstract class. Not sure if that is a better situation or not - it does make it more flexible though.
There was a problem hiding this comment.
That's not a bad idea actually -- would require a lot of code changes though ...
There was a problem hiding this comment.
i.e. we should change all the places that accept SparkListener to accept the SparkListenerInterface
|
Just that minor question but otherwise LGTM |
|
OK updated. |
| * Simple SparkListener that logs a few summary statistics when each stage completes. | ||
| */ | ||
| @DeveloperApi | ||
| class StatsReportListener extends SparkListener with Logging { |
There was a problem hiding this comment.
this was simply moved from one file into a new file.
|
Test build #2736 has finished for PR 12142 at commit
|
|
Test build #54831 has finished for PR 12142 at commit
|
|
Looks good. This will break cases where the user has a class that listens for Spark events but already extends some parent, but that's probably OK. |
|
Ok merged into master. |
Currently all `SparkFirehoseListener` implementations are broken since we expect listeners to extend `SparkListener`, while the fire hose only extends `SparkListenerInterface`. This changes the addListener function and the config based injection to use the interface instead. The existing tests in SparkListenerSuite are improved such that they would have caught this. Follow-up to #12142 Author: Michael Armbrust <michael@databricks.com> Closes #12227 from marmbrus/fixListener.
What changes were proposed in this pull request?
Scala traits are difficult to maintain binary compatibility on, and as a result we had to introduce JavaSparkListener. In Spark 2.0 we can change SparkListener from a trait to an abstract class and then remove JavaSparkListener.
How was this patch tested?
Updated related unit tests.