[SPARK-20048][SQL] Cloning SessionState does not clone query execution listeners #17379

kunalkhamar · 2017-03-21T21:19:25Z

What changes were proposed in this pull request?

Bugfix from SPARK-19540.
Cloning SessionState does not clone query execution listeners, so cloned session is unable to listen to events on queries.

How was this patch tested?

Unit test

SparkQA · 2017-03-21T22:52:07Z

Test build #75002 has finished for PR 17379 at commit ad77fe9.

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

kunalkhamar · 2017-03-21T23:59:56Z

retest this please

SparkQA · 2017-03-22T02:14:03Z

Test build #75015 has finished for PR 17379 at commit ad77fe9.

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

SparkQA · 2017-03-23T00:37:40Z

Test build #75066 has finished for PR 17379 at commit f63e81d.

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

tdas

overall, i am confused as to why listener is not being passed as a constructor param, rather as a complicated override.

And why are docs being removed?

tdas · 2017-03-23T21:17:40Z

sql/core/src/main/scala/org/apache/spark/sql/internal/SessionState.scala

why have you removed these?

These comments are adding little or no value. We should remove or make them more detailed, which would you prefer? If the latter, what's a good doc for shared state and experimental methods?

Added it back, hopefully more descriptive.

tdas · 2017-03-23T21:19:05Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveSessionState.scala

why have these been removed?

Each of these is identical to their SessionState counterpart and should be inherited by Scaladoc comment inheritance, do we still need them?

HiveSessionState is gone, so this is not relevant anymore.

tdas · 2017-03-23T21:28:17Z

sql/core/src/main/scala/org/apache/spark/sql/internal/SessionState.scala

Why do it this way? Why not pass it as a constructor param?

I think the general rule we followed in the cloneSession PR is that vals that directly depend on SparkSession to be initialized, are to be constructor params. We leave the other vals in the body of class.
The advantage here is less duplicated code between SessionState, HiveSessionState and TestHiveSparkSession.
This is consistent with that, what would be the advantage of changing it to be a param?

Builders were added in SPARK-20100. Changed initialization to use that pattern, so it is a constructor param now.

tdas · 2017-03-23T21:29:00Z

sql/core/src/test/scala/org/apache/spark/sql/SessionStateSuite.scala

Add a comment on what it creates? what the two things returned.

super nit: cleaner to do

class CommandCollector extends QueryExecutionListener { val commands = ... ... }

neat, changed.

SparkQA · 2017-03-24T03:59:01Z

Test build #75135 has finished for PR 17379 at commit eb5581a.

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

SparkQA · 2017-03-28T22:41:36Z

Test build #75328 has finished for PR 17379 at commit 16f5bea.

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

SparkQA · 2017-03-29T01:48:56Z

Test build #75331 has finished for PR 17379 at commit cad1b63.

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

tdas

Few minor comments.
@hvanhovell should take a look too.

tdas · 2017-03-28T23:29:18Z

sql/core/src/main/scala/org/apache/spark/sql/internal/SessionState.scala

    val conf: SQLConf,
    val experimentalMethods: ExperimentalMethods,
    val functionRegistry: FunctionRegistry,
+    val udf: UDFRegistration,


udf -> udfRegistration

tdas · 2017-03-29T01:48:47Z

sql/core/src/main/scala/org/apache/spark/sql/internal/sessionStateBuilders.scala

+   *
+   * Note 1: The user-defined functions must be deterministic.
+   * Note 2: This depends on the `functionRegistry` field.
+   */


This file only contains effectively one builder. So it should be named after the class.

It also contains a mix-in WithTestConf. But renaming is fine.

tdas · 2017-03-29T01:58:07Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveSessionState.scala

+ * Builder that produces a Hive aware [[SessionState]].
 */
 @Experimental
 @InterfaceStability.Unstable


This file should not be named HiveSessionState anymore. It doesnt even have the class HiveSessionState. It does have an object HiveSession, but do we need that object any more?
@hvanhovell

Renamed, removed object HiveSessionState.

Yeah, lets rename the file to HiveSessionBuilder.scala

SparkQA · 2017-03-29T06:00:53Z

Test build #75337 has finished for PR 17379 at commit be191c6.

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

SparkQA · 2017-03-29T06:01:44Z

Test build #75338 has finished for PR 17379 at commit 119dae9.

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

kunalkhamar · 2017-03-29T06:18:09Z

cc @hvanhovell

hvanhovell · 2017-03-29T12:27:56Z

sql/core/src/main/scala/org/apache/spark/sql/internal/BaseSessionStateBuilder.scala

+   *
+   * Note 1: The user-defined functions must be deterministic.
+   * Note 2: This depends on the `functionRegistry` field.
+   */


Minor: I was wondering why you moved this into the builder?

It was the only thing not initialized in the builder, thought it would be more consistent to have all initialization in the builder. Is this okay?

hvanhovell

@kunalkhamar could you rename HiveSessionState into HiveSessionStateBuilder? Other than that, LGTM.

kunalkhamar · 2017-03-29T18:12:50Z

@hvanhovell cool, it should be HiveSessionStateBuilder now.

hvanhovell · 2017-03-29T19:34:39Z

Merging to master. Thanks!

kunalkhamar mentioned this pull request Mar 21, 2017

[SPARK-19540][SQL] Add ability to clone SparkSession wherein cloned session has an identical copy of the SessionState #16826

Closed

tdas suggested changes Mar 23, 2017

View reviewed changes

Update listenerManager cloning.

16f5bea

kunalkhamar force-pushed the clone-bugfix branch from eb5581a to 16f5bea Compare March 28, 2017 20:36

nits - remove remaining traces of HiveSessionState.

cad1b63

tdas reviewed Mar 29, 2017

View reviewed changes

kunalkhamar added 2 commits March 28, 2017 20:42

Change reflection to init SessionState, remove apply.

be191c6

nits

119dae9

hvanhovell reviewed Mar 29, 2017

View reviewed changes

hvanhovell approved these changes Mar 29, 2017

View reviewed changes

asfgit closed this in 142f6d1 Mar 29, 2017

[SPARK-20048][SQL] Cloning SessionState does not clone query execution listeners #17379

[SPARK-20048][SQL] Cloning SessionState does not clone query execution listeners #17379

Uh oh!

Conversation

kunalkhamar commented Mar 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Mar 21, 2017

Uh oh!

kunalkhamar commented Mar 21, 2017

Uh oh!

SparkQA commented Mar 22, 2017

Uh oh!

SparkQA commented Mar 23, 2017

Uh oh!

tdas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kunalkhamar Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kunalkhamar Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 24, 2017

Uh oh!

SparkQA commented Mar 28, 2017

Uh oh!

SparkQA commented Mar 29, 2017

Uh oh!

tdas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 29, 2017

Uh oh!

SparkQA commented Mar 29, 2017

Uh oh!

kunalkhamar commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

kunalkhamar commented Mar 21, 2017 •

edited

Loading

kunalkhamar Mar 28, 2017 •

edited

Loading

kunalkhamar Mar 28, 2017 •

edited

Loading

kunalkhamar commented Mar 29, 2017 •

edited

Loading