[SPARK-32165][SQL] Ensure Spark only initiates SharedState once across SparkSessions by Ngone51 · Pull Request #31053 · apache/spark

Ngone51 · 2021-01-06T04:20:46Z

What changes were proposed in this pull request?

This PR makes sure that SharedState would be only initiated once across SparkSessions.

Why are the changes needed?

This helps us to fix the listener(e.g., SQLAppStatusListener) leak issue, and allow SparkSessions created by getOrCreate to share the CacheManager, global temp view, etc.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Added UTs.

Ngone51 · 2021-01-06T04:22:21Z

@cloud-fan @vinooganesh @advancedxy Please take a look, thanks!

SparkQA · 2021-01-06T06:00:43Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38301/

SparkQA · 2021-01-06T06:05:06Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38301/

SparkQA · 2021-01-06T08:05:50Z

Test build #133713 has finished for PR 31053 at commit 38379d0.

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

cloud-fan · 2021-01-06T08:08:41Z

seems we can also fix this issue by making sure there is only one SharedState instance. cc @yaooqinn

yaooqinn · 2021-01-06T08:19:36Z

seems we can also fix this issue by making sure there is only one SharedState instance. cc @yaooqinn

right

Ngone51 · 2021-01-07T03:03:36Z

seems we can also fix this issue by making sure there is only one SharedState instance.

Seems like the SharedState, currently, is only shared between parent and child sessions. e.g., newSession() or cloneSession()...

If we want to make sure there's only one SharedState instance in this case, we need to make SparkSession.builder.getOrCreate to share the SharedState instance between the sibling sessions. Would it be ok?

Besides, although the only one SharedState instance can fixe SQLAppStatusListener leak, it leaves the ApplicationEndListener being fixed in another way(#28128). And addToQueueIfNotExists() could fix them in a more general way.

Thoughts?

cloud-fan · 2021-01-07T14:16:21Z

I think it's an accident that the SharedState can be created more than once, as it holds the ExternalCatalog instance. If we fix that first, then this PR is just a refactor, as it doesn't fix any issues.

vinooganesh · 2021-01-07T14:45:54Z

I agree with @cloud-fan and think it's easier just to ensure that SharedState can only be created once. Trying to compare listeners with their fully qualified class name seems more problematic than necessary (especially when there are so many different workflows that people use when setting up their listeners - including ones where different instances of a listener with the same fully qualified class name exist)

Ngone51 · 2021-01-13T09:23:57Z

Hey guys, I've updated the PR with the one SharedState way. Please take another look, thanks!

SparkQA · 2021-01-13T10:29:16Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38595/

SparkQA · 2021-01-13T11:01:49Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38595/

SparkQA · 2021-01-13T13:25:34Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38600/

SparkQA · 2021-01-13T13:59:06Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38600/

SparkQA · 2021-01-13T14:34:15Z

Test build #134013 has finished for PR 31053 at commit 0d4c7d1.

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

SparkQA · 2021-01-13T18:02:21Z

Test build #134007 has finished for PR 31053 at commit 88bbd39.

This patch fails from timeout after a configured wait of 500m.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-01-14T03:35:14Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38622/

SparkQA · 2021-01-14T04:03:47Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38622/

cloud-fan · 2021-01-14T04:30:55Z

is it duplicated with checking assert(countListener("ExecutionListenerBus", context) === 2)?

No. Here we want to ensure other potential listeners won't leak across SparkSessions.

jaceklaskowski

LGTM (non-binding)

SparkQA · 2021-01-14T10:36:48Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38637/

SparkQA · 2021-01-14T10:58:21Z

Test build #134035 has finished for PR 31053 at commit 4bc644d.

This patch fails from timeout after a configured wait of 500m.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-01-16T04:36:37Z

Test build #134134 has finished for PR 31053 at commit 9c32d18.

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

SparkQA · 2021-01-16T04:40:04Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38717/

SparkQA · 2021-01-16T05:28:24Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38717/

Ngone51 · 2021-01-19T02:24:27Z

retest this please

SparkQA · 2021-01-19T03:44:36Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38795/

SparkQA · 2021-01-19T03:45:33Z

Test build #134210 has finished for PR 31053 at commit 9c32d18.

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

SparkQA · 2021-01-19T03:50:08Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38795/

Ngone51 · 2021-01-20T02:48:25Z

retest this please

SparkQA · 2021-01-20T03:54:13Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38832/

SparkQA · 2021-01-20T04:04:57Z

Test build #134247 has finished for PR 31053 at commit 9c32d18.

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

maropu · 2021-01-20T04:11:15Z

retest this please

SparkQA · 2021-01-20T04:46:53Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38832/

SparkQA · 2021-01-20T05:52:36Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38839/

SparkQA · 2021-01-20T06:28:08Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38839/

SparkQA · 2021-01-20T13:22:31Z

Test build #134253 has finished for PR 31053 at commit 9c32d18.

This patch fails from timeout after a configured wait of 500m.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2021-01-20T13:53:16Z

retest this please

SparkQA · 2021-01-20T14:50:12Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38863/

SparkQA · 2021-01-20T15:18:42Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/38863/

SparkQA · 2021-01-20T22:25:31Z

Test build #134275 has finished for PR 31053 at commit 9c32d18.

This patch fails from timeout after a configured wait of 500m.
This patch merges cleanly.
This patch adds no public classes.

github-actions · 2021-05-01T00:10:33Z

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

dnskr · 2022-01-16T16:55:10Z

@Ngone51 @cloud-fan Hi! The issue still exists. Can we reopen this PR? Otherwise please provide any comments why this cannot be merged or what should be done first?

github-actionsBot added CORE SQL labels Jan 6, 2021

tgravescs reviewed Jan 6, 2021
View reviewed changes

Comment threadsql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala Outdated
Comment threadsql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala Outdated

Ngone51 force-pushed the spark-32165 branch from 38379d0 to 88bbd39Compare January 13, 2021 09:14

Ngone51 changed the title ~~[SPARK-32165][SQL][CORE] Add addToQueueIfNotExists to ensure some SQL listeners only register once across SparkSessions~~ [SPARK-32165][SQL] Ensure Spark only initiates SharedState once across SparkSessionsJan 13, 2021

jaceklaskowski reviewed Jan 13, 2021
View reviewed changes

Comment threadsql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala Outdated
Comment threadsql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala Outdated

vinooganesh reviewed Jan 13, 2021
View reviewed changes

Comment threadsql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala Outdated

cloud-fan reviewed Jan 14, 2021
View reviewed changes

jaceklaskowski reviewed Jan 14, 2021
View reviewed changes

Comment threadsql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala Outdated

jaceklaskowski approved these changes Jan 14, 2021
View reviewed changes

remove comment
9c32d18

Ngone51 force-pushed the spark-32165 branch from 30cd50d to 9c32d18Compare January 16, 2021 02:57

Ngone51 closed this Jan 20, 2021

Ngone51 reopened this Jan 20, 2021

Ngone51 closed this Jan 20, 2021

Ngone51 reopened this Jan 20, 2021

Ngone51 mentioned this pull request Jan 21, 2021
[SPARK-32165][SQL] Ensure Spark only initiates SharedState once across SparkSessions #31268
Closed

github-actionsBot added the Stale label May 1, 2021

github-actionsBot closed this May 2, 2021

Uh oh!

Conversation

Ngone51 commented Jan 6, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Ngone51 commented Jan 6, 2021

Uh oh!

SparkQA commented Jan 6, 2021

Uh oh!

SparkQA commented Jan 6, 2021

Uh oh!

SparkQA commented Jan 6, 2021

Uh oh!

cloud-fan commented Jan 6, 2021

Uh oh!

yaooqinn commented Jan 6, 2021

Uh oh!

Uh oh!

Uh oh!

Ngone51 commented Jan 7, 2021

Uh oh!

cloud-fan commented Jan 7, 2021

Uh oh!

vinooganesh commented Jan 7, 2021

Uh oh!

Ngone51 commented Jan 13, 2021

Uh oh!

SparkQA commented Jan 13, 2021

Uh oh!

SparkQA commented Jan 13, 2021

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Jan 13, 2021

Uh oh!

SparkQA commented Jan 13, 2021

Uh oh!

Uh oh!

SparkQA commented Jan 13, 2021

Uh oh!

SparkQA commented Jan 13, 2021

Uh oh!

SparkQA commented Jan 14, 2021

Uh oh!

SparkQA commented Jan 14, 2021

Uh oh!

cloud-fanJan 14, 2021

Choose a reason for hiding this comment

Uh oh!

Ngone51Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jaceklaskowski left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 14, 2021

Uh oh!

SparkQA commented Jan 14, 2021

Uh oh!

SparkQA commented Jan 16, 2021

Uh oh!

SparkQA commented Jan 16, 2021

Uh oh!

SparkQA commented Jan 16, 2021

Uh oh!

Ngone51 commented Jan 19, 2021

Uh oh!

SparkQA commented Jan 19, 2021

Uh oh!

SparkQA commented Jan 19, 2021

Uh oh!

SparkQA commented Jan 19, 2021

Uh oh!

Ngone51 commented Jan 20, 2021

Uh oh!

Ngone51 commented Jan 6, 2021•
edited
Loading