Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-19558][sql] Add config key to register QueryExecutionListeners automatically. #19309

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
4 participants
@vanzin
Copy link
Contributor

commented Sep 21, 2017

This change adds a new SQL config key that is equivalent to SparkContext's
"spark.extraListeners", allowing users to register QueryExecutionListener
instances through the Spark configuration system instead of having to
explicitly do it in code.

The code used by SparkContext to implement the feature was refactored into
a helper method in the Utils class, and SQL's ExecutionListenerManager was
modified to use it to initialize listener declared in the configuration.

Unit tests were added to verify all the new functionality.

[SPARK-19558][sql] Add config key to register QueryExecutionListeners…
… automatically.

This change adds a new SQL config key that is equivalent to SparkContext's
"spark.extraListeners", allowing users to register QueryExecutionListener
instances through the Spark configuration system instead of having to
explicitly do it in code.

The code used by SparkContext to implement the feature was refactored into
a helper method in the Utils class, and SQL's ExecutionListenerManager was
modified to use it to initialize listener declared in the configuration.

Unit tests were added to verify all the new functionality.
@SparkQA

This comment has been minimized.

Copy link

commented Sep 21, 2017

Test build #82041 has finished for PR 19309 at commit c52fd71.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • s\"$name is not a subclass of $
  • .doc(\"List of class names implementing QueryExecutionListener that will be automatically \" +
@vanzin

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2017

@squito

squito approved these changes Sep 28, 2017

Copy link
Contributor

left a comment

lgtm, a small question on the UnsupportedOperationException handling.


case e: InvocationTargetException =>
e.getCause() match {
case _: UnsupportedOperationException =>

This comment has been minimized.

Copy link
@squito

squito Sep 28, 2017

Contributor

I am a bit worried catching UnsupportedOperationException is a bit too general ... what if its thrown by something else deep inside the extension class? The user won't get to see what happened.

OTOH, I can't think of anything better to catch (doesn't seem worth a special exception type). Maybe log the entire exception at debug level?

This comment has been minimized.

Copy link
@vanzin

vanzin Sep 28, 2017

Author Contributor

I wanted to avoid adding a special new exception for this. I'll add the log.

@SparkQA

This comment has been minimized.

Copy link

commented Sep 28, 2017

Test build #82287 has finished for PR 19309 at commit bcc44e9.

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

This comment has been minimized.

Copy link
Member

commented Sep 29, 2017

Will review it this weekend.


test("register query execution listeners using configuration") {
val conf = new SparkConf(false)
.set(QUERY_EXECUTION_LISTENERS, Seq(classOf[CountingQueryExecutionListener].getName()))

This comment has been minimized.

Copy link
@gatorsmile

gatorsmile Oct 8, 2017

Member

Now, this test only has one single listener whose constructor has zero arg. Could you add one more with a SparkConf arg in the constructor?

This comment has been minimized.

Copy link
@vanzin

vanzin Oct 9, 2017

Author Contributor

That's covered by UtilsSuite already.

@gatorsmile

This comment has been minimized.

Copy link
Member

commented Oct 10, 2017

retest this please

@gatorsmile

This comment has been minimized.

Copy link
Member

commented Oct 10, 2017

Let me summarize the PR. Please correct me if anything is missing.

Background

Currently, our users can register a listener one by one during the executions by calling the SparkContext API:

def addSparkListener(listener: SparkListenerInterface)

Users also can specify them through the Conf spark.extraListeners. SparkContext will automatically register the extra listeners during the initialization phase.

In Spark SQL, QueryExecutionListener enables users to register QueryExecutionListener by calling the API

def register(listener: QueryExecutionListener)

Proposal

This PR is proposing to add a similar static SQLConf spark.sql.queryExecutionListeners. When a Spark session is built/cloned, the query execution listeners specified in the new conf will be registered automatically.

@vanzin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2017

That's basically what the PR summary says.

@SparkQA

This comment has been minimized.

Copy link

commented Oct 10, 2017

Test build #82597 has finished for PR 19309 at commit bcc44e9.

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

This comment has been minimized.

Copy link
Member

commented Oct 10, 2017

LGTM

@gatorsmile

This comment has been minimized.

Copy link
Member

commented Oct 10, 2017

Thanks! Merged to master.

@asfgit asfgit closed this in bd4eb9c Oct 10, 2017

@vanzin vanzin deleted the vanzin:SPARK-19558 branch Oct 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.