You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "ever4Kenny (via GitHub)" <gi...@apache.org> on 2026/02/04 06:30:13 UTC
[PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
ever4Kenny opened a new pull request, #54133:
URL: https://github.com/apache/spark/pull/54133
### What changes were proposed in this pull request?
This patch adds a new static configuration `spark.sql.ui.appStatusListener.enabled` to control whether SQLAppStatusListener is loaded during SharedState initialization.
### Why are the changes needed?
Complex SQL queries with many stages and tasks cause driver OOM due to excessive AccumulatorMetadata objects held by SQLAppStatusListener.
**Memory regression from Spark 3.2 to 3.5:**
- Accumulator count per task increased 4x due to new metrics (SPARK-36620, SPARK-40711, SPARK-43214)
- Example: 155 stages × 1000 tasks × 100 accumulators = 15.5M objects (~1.5GB driver memory)
- Same query runs on 3.2.2 with 2GB driver but OOMs on 3.5.7 with 8GB driver
### Does this PR introduce any user-facing change?
Yes. A new static configuration `spark.sql.ui.appStatusListener.enabled` (default: true) is added.
When set to false:
- SQLAppStatusListener is not created, significantly reducing driver memory usage
- Limited SQL execution metrics in live Spark UI
- Event logs are still written for History Server analysis
### How was this patch tested?
- Added unit tests in SharedStateSuite to verify SQLAppStatusListener creation behavior
- Tested with production workload: OOM resolved when config is disabled
### Was this patch authored or co-authored using generative AI tooling?
No.
---
I affirm that the contribution is my original work and that I license the work to the project under the project's open source license.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Disable SQLAppStatusListener when UI is disabled [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #54133:
URL: https://github.com/apache/spark/pull/54133#issuecomment-4805526151
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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2787056530
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala:
##########
@@ -236,6 +236,15 @@ object StaticSQLConf {
.booleanConf
.createWithDefault(false)
+ val SQL_UI_APP_STATUS_LISTENER_ENABLED =
+ buildStaticConf("spark.sql.ui.appStatusListener.enabled")
+ .doc("Whether to enable SQLAppStatusListener for collecting SQL execution metrics. " +
+ "Disabling this can significantly reduce driver memory usage for complex queries with " +
+ "many stages and tasks, but will result in limited SQL execution information in the UI.")
+ .version("4.0.0")
+ .booleanConf
+ .createWithDefault(true)
Review Comment:
Spark currently under 4.2 development cycle so version should be "4.2.0".
I agree `appStatusListener` should keep enabled by default. But could you disable it and tigger a round CI, to help us evalute the impact?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2810502003
##########
sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala:
##########
@@ -120,11 +120,17 @@ private[sql] class SharedState(
*/
val statusStore: SQLAppStatusStore = {
val kvStore = sparkContext.statusStore.store.asInstanceOf[ElementTrackingStore]
- val listener = new SQLAppStatusListener(conf, kvStore, live = true)
- sparkContext.listenerBus.addToStatusQueue(listener)
- val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
- sparkContext.ui.foreach(new SQLTab(statusStore, _))
- statusStore
+ // Only create SQLAppStatusListener when explicitly enabled to avoid memory overhead
+ if (conf.get(SQL_UI_APP_STATUS_LISTENER_ENABLED)) {
+ val listener = new SQLAppStatusListener(conf, kvStore, live = true)
+ sparkContext.listenerBus.addToStatusQueue(listener)
+ val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
+ sparkContext.ui.foreach(new SQLTab(statusStore, _))
+ statusStore
+ } else {
+ // When disabled, create a minimal status store without listener
+ new SQLAppStatusStore(kvStore, None)
+ }
Review Comment:
If this is specific to UI, control it using `spark.ui.enabled` and avoid introducing a config ? Like `streamingQueryStatusListener` below ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2810502003
##########
sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala:
##########
@@ -120,11 +120,17 @@ private[sql] class SharedState(
*/
val statusStore: SQLAppStatusStore = {
val kvStore = sparkContext.statusStore.store.asInstanceOf[ElementTrackingStore]
- val listener = new SQLAppStatusListener(conf, kvStore, live = true)
- sparkContext.listenerBus.addToStatusQueue(listener)
- val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
- sparkContext.ui.foreach(new SQLTab(statusStore, _))
- statusStore
+ // Only create SQLAppStatusListener when explicitly enabled to avoid memory overhead
+ if (conf.get(SQL_UI_APP_STATUS_LISTENER_ENABLED)) {
+ val listener = new SQLAppStatusListener(conf, kvStore, live = true)
+ sparkContext.listenerBus.addToStatusQueue(listener)
+ val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
+ sparkContext.ui.foreach(new SQLTab(statusStore, _))
+ statusStore
+ } else {
+ // When disabled, create a minimal status store without listener
+ new SQLAppStatusStore(kvStore, None)
+ }
Review Comment:
If this is specific to UI, control it using `spark.ui.enabled` ? Like `streamingQueryStatusListener` below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2850558585
##########
sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala:
##########
@@ -120,11 +120,17 @@ private[sql] class SharedState(
*/
val statusStore: SQLAppStatusStore = {
val kvStore = sparkContext.statusStore.store.asInstanceOf[ElementTrackingStore]
- val listener = new SQLAppStatusListener(conf, kvStore, live = true)
- sparkContext.listenerBus.addToStatusQueue(listener)
- val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
- sparkContext.ui.foreach(new SQLTab(statusStore, _))
- statusStore
+ // Only create SQLAppStatusListener when explicitly enabled to avoid memory overhead
+ if (conf.get(SQL_UI_APP_STATUS_LISTENER_ENABLED)) {
+ val listener = new SQLAppStatusListener(conf, kvStore, live = true)
+ sparkContext.listenerBus.addToStatusQueue(listener)
+ val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
+ sparkContext.ui.foreach(new SQLTab(statusStore, _))
+ statusStore
+ } else {
+ // When disabled, create a minimal status store without listener
+ new SQLAppStatusStore(kvStore, None)
+ }
Review Comment:
I'm a little confused, I believe the author means: UI almost works even if appStatusListener is disabled.
If so, separate configs make sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2787056530
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala:
##########
@@ -236,6 +236,15 @@ object StaticSQLConf {
.booleanConf
.createWithDefault(false)
+ val SQL_UI_APP_STATUS_LISTENER_ENABLED =
+ buildStaticConf("spark.sql.ui.appStatusListener.enabled")
+ .doc("Whether to enable SQLAppStatusListener for collecting SQL execution metrics. " +
+ "Disabling this can significantly reduce driver memory usage for complex queries with " +
+ "many stages and tasks, but will result in limited SQL execution information in the UI.")
+ .version("4.0.0")
+ .booleanConf
+ .createWithDefault(true)
Review Comment:
Spark currently under 4.2 development cycle so version should be "4.2.0".
I agree `appStatusListener` should keep enabled by default. But could you disable it and tigger a round CI, to help us evaluate the impact?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "ever4Kenny (via GitHub)" <gi...@apache.org>.
ever4Kenny commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2851103998
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala:
##########
@@ -236,6 +236,15 @@ object StaticSQLConf {
.booleanConf
.createWithDefault(false)
+ val SQL_UI_APP_STATUS_LISTENER_ENABLED =
+ buildStaticConf("spark.sql.ui.appStatusListener.enabled")
+ .doc("Whether to enable SQLAppStatusListener for collecting SQL execution metrics. " +
+ "Disabling this can significantly reduce driver memory usage for complex queries with " +
+ "many stages and tasks, but will result in limited SQL execution information in the UI.")
+ .version("4.0.0")
+ .booleanConf
+ .createWithDefault(true)
Review Comment:
Few metrics related tests failed with the listener disabled, could you please take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "ever4Kenny (via GitHub)" <gi...@apache.org>.
ever4Kenny commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2852103468
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala:
##########
@@ -236,6 +236,15 @@ object StaticSQLConf {
.booleanConf
.createWithDefault(false)
+ val SQL_UI_APP_STATUS_LISTENER_ENABLED =
+ buildStaticConf("spark.sql.ui.appStatusListener.enabled")
Review Comment:
Ok, directly use spark.ui.enabled to toggle the lisener.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "ever4Kenny (via GitHub)" <gi...@apache.org>.
ever4Kenny commented on PR #54133:
URL: https://github.com/apache/spark/pull/54133#issuecomment-3875133052
> > SQLAppStatusListener is not created, significantly reducing driver memory usage
>
> How much memory decreased in your case?
>
> > Limited SQL execution metrics in live Spark UI
>
> Do you have a concrete list of the impact?
1. At least 6GB memory was decreased(with listener 8G driver oom, w/o listener 2G driver ran fine)
2. In my case the SQL tab still existed and the DAG and the metrics were still intact both in live UI or history UI(i hardly notice any change)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2810502003
##########
sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala:
##########
@@ -120,11 +120,17 @@ private[sql] class SharedState(
*/
val statusStore: SQLAppStatusStore = {
val kvStore = sparkContext.statusStore.store.asInstanceOf[ElementTrackingStore]
- val listener = new SQLAppStatusListener(conf, kvStore, live = true)
- sparkContext.listenerBus.addToStatusQueue(listener)
- val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
- sparkContext.ui.foreach(new SQLTab(statusStore, _))
- statusStore
+ // Only create SQLAppStatusListener when explicitly enabled to avoid memory overhead
+ if (conf.get(SQL_UI_APP_STATUS_LISTENER_ENABLED)) {
+ val listener = new SQLAppStatusListener(conf, kvStore, live = true)
+ sparkContext.listenerBus.addToStatusQueue(listener)
+ val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
+ sparkContext.ui.foreach(new SQLTab(statusStore, _))
+ statusStore
+ } else {
+ // When disabled, create a minimal status store without listener
+ new SQLAppStatusStore(kvStore, None)
+ }
Review Comment:
If this is specific to UI, control it using `spark.ui.enabled` and avoid introducing a config ? Similar (as a pattern) to `streamingQueryStatusListener` below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "ever4Kenny (via GitHub)" <gi...@apache.org>.
ever4Kenny commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2851113947
##########
sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala:
##########
@@ -120,11 +120,17 @@ private[sql] class SharedState(
*/
val statusStore: SQLAppStatusStore = {
val kvStore = sparkContext.statusStore.store.asInstanceOf[ElementTrackingStore]
- val listener = new SQLAppStatusListener(conf, kvStore, live = true)
- sparkContext.listenerBus.addToStatusQueue(listener)
- val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
- sparkContext.ui.foreach(new SQLTab(statusStore, _))
- statusStore
+ // Only create SQLAppStatusListener when explicitly enabled to avoid memory overhead
+ if (conf.get(SQL_UI_APP_STATUS_LISTENER_ENABLED)) {
+ val listener = new SQLAppStatusListener(conf, kvStore, live = true)
+ sparkContext.listenerBus.addToStatusQueue(listener)
+ val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
+ sparkContext.ui.foreach(new SQLTab(statusStore, _))
+ statusStore
+ } else {
+ // When disabled, create a minimal status store without listener
+ new SQLAppStatusStore(kvStore, None)
+ }
Review Comment:
> I'm a little confused, I believe the author means: UI almost works even if appStatusListener is disabled.
>
> If so, separate configs make sense.
Yes that's true, the UI is still available without the listener.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "ever4Kenny (via GitHub)" <gi...@apache.org>.
ever4Kenny commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2850511575
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala:
##########
@@ -236,6 +236,15 @@ object StaticSQLConf {
.booleanConf
.createWithDefault(false)
+ val SQL_UI_APP_STATUS_LISTENER_ENABLED =
+ buildStaticConf("spark.sql.ui.appStatusListener.enabled")
+ .doc("Whether to enable SQLAppStatusListener for collecting SQL execution metrics. " +
+ "Disabling this can significantly reduce driver memory usage for complex queries with " +
+ "many stages and tasks, but will result in limited SQL execution information in the UI.")
+ .version("4.0.0")
+ .booleanConf
+ .createWithDefault(true)
Review Comment:
Ok, I will try disable it and trigger a CI, and enable it afterwards.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2851217570
##########
sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala:
##########
@@ -120,11 +120,17 @@ private[sql] class SharedState(
*/
val statusStore: SQLAppStatusStore = {
val kvStore = sparkContext.statusStore.store.asInstanceOf[ElementTrackingStore]
- val listener = new SQLAppStatusListener(conf, kvStore, live = true)
- sparkContext.listenerBus.addToStatusQueue(listener)
- val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
- sparkContext.ui.foreach(new SQLTab(statusStore, _))
- statusStore
+ // Only create SQLAppStatusListener when explicitly enabled to avoid memory overhead
+ if (conf.get(SQL_UI_APP_STATUS_LISTENER_ENABLED)) {
+ val listener = new SQLAppStatusListener(conf, kvStore, live = true)
+ sparkContext.listenerBus.addToStatusQueue(listener)
+ val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
+ sparkContext.ui.foreach(new SQLTab(statusStore, _))
+ statusStore
+ } else {
+ // When disabled, create a minimal status store without listener
+ new SQLAppStatusStore(kvStore, None)
+ }
Review Comment:
the impacts looks fine! (you may need to rebase your branch to the latest master to solve the Python failures)
So how about
1. mark the new config `spark.sql.ui.appStatusListener.enabled` as `internal`, to make it for advanced users only, as it has side effects that might confuse normal users.
2. keep the default value fallback to `spark.ui.enabled`, then disabling UI also reduces driver memory usage.
@mridulm @cloud-fan, WDYT? Does disabling `appStatusListener` have other potential impacts other than UI?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "ever4Kenny (via GitHub)" <gi...@apache.org>.
ever4Kenny commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2852107978
##########
sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala:
##########
@@ -120,11 +120,17 @@ private[sql] class SharedState(
*/
val statusStore: SQLAppStatusStore = {
val kvStore = sparkContext.statusStore.store.asInstanceOf[ElementTrackingStore]
- val listener = new SQLAppStatusListener(conf, kvStore, live = true)
- sparkContext.listenerBus.addToStatusQueue(listener)
- val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
- sparkContext.ui.foreach(new SQLTab(statusStore, _))
- statusStore
+ // Only create SQLAppStatusListener when explicitly enabled to avoid memory overhead
+ if (conf.get(SQL_UI_APP_STATUS_LISTENER_ENABLED)) {
+ val listener = new SQLAppStatusListener(conf, kvStore, live = true)
+ sparkContext.listenerBus.addToStatusQueue(listener)
+ val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
+ sparkContext.ui.foreach(new SQLTab(statusStore, _))
+ statusStore
+ } else {
+ // When disabled, create a minimal status store without listener
+ new SQLAppStatusStore(kvStore, None)
+ }
Review Comment:
Thanks for your help!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Disable SQLAppStatusListener when UI is disabled [spark]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2852191969
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala:
##########
@@ -236,6 +236,15 @@ object StaticSQLConf {
.booleanConf
.createWithDefault(false)
+ val SQL_UI_APP_STATUS_LISTENER_ENABLED =
+ buildStaticConf("spark.sql.ui.appStatusListener.enabled")
Review Comment:
I believe what @cloud-fan said "disable SQL UI" means "disable SQLTab", not the whole live UI.
You probably need a new config like `spark.sql.ui.enabled`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "ever4Kenny (via GitHub)" <gi...@apache.org>.
ever4Kenny commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2850542389
##########
sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala:
##########
@@ -120,11 +120,17 @@ private[sql] class SharedState(
*/
val statusStore: SQLAppStatusStore = {
val kvStore = sparkContext.statusStore.store.asInstanceOf[ElementTrackingStore]
- val listener = new SQLAppStatusListener(conf, kvStore, live = true)
- sparkContext.listenerBus.addToStatusQueue(listener)
- val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
- sparkContext.ui.foreach(new SQLTab(statusStore, _))
- statusStore
+ // Only create SQLAppStatusListener when explicitly enabled to avoid memory overhead
+ if (conf.get(SQL_UI_APP_STATUS_LISTENER_ENABLED)) {
+ val listener = new SQLAppStatusListener(conf, kvStore, live = true)
+ sparkContext.listenerBus.addToStatusQueue(listener)
+ val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
+ sparkContext.ui.foreach(new SQLTab(statusStore, _))
+ statusStore
+ } else {
+ // When disabled, create a minimal status store without listener
+ new SQLAppStatusStore(kvStore, None)
+ }
Review Comment:
Good point, I will use "spark.ui.enabled" instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "ever4Kenny (via GitHub)" <gi...@apache.org>.
ever4Kenny commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2850511575
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala:
##########
@@ -236,6 +236,15 @@ object StaticSQLConf {
.booleanConf
.createWithDefault(false)
+ val SQL_UI_APP_STATUS_LISTENER_ENABLED =
+ buildStaticConf("spark.sql.ui.appStatusListener.enabled")
+ .doc("Whether to enable SQLAppStatusListener for collecting SQL execution metrics. " +
+ "Disabling this can significantly reduce driver memory usage for complex queries with " +
+ "many stages and tasks, but will result in limited SQL execution information in the UI.")
+ .version("4.0.0")
+ .booleanConf
+ .createWithDefault(true)
Review Comment:
Ok, I will try enable it and trigger a CI, and disable it afterwards.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #54133:
URL: https://github.com/apache/spark/pull/54133#issuecomment-3845617367
## JIRA Issue Information
=== Improvement SPARK-55353 ===
Summary: Driver OOM regression with complex SQL queries: Add config to disable SQLAppStatusListener
Assignee: None
Status: Open
Affected: ["3.5.7"]
---
*This comment was automatically generated by GitHub Actions*
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #54133:
URL: https://github.com/apache/spark/pull/54133#issuecomment-3857722200
> SQLAppStatusListener is not created, significantly reducing driver memory usage
How much memory decreased in your case?
> Limited SQL execution metrics in live Spark UI
Do you have a concrete list of the impact?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Disable SQLAppStatusListener when UI is disabled [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #54133: [SPARK-55353][SQL] Disable SQLAppStatusListener when UI is disabled
URL: https://github.com/apache/spark/pull/54133
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2851217570
##########
sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala:
##########
@@ -120,11 +120,17 @@ private[sql] class SharedState(
*/
val statusStore: SQLAppStatusStore = {
val kvStore = sparkContext.statusStore.store.asInstanceOf[ElementTrackingStore]
- val listener = new SQLAppStatusListener(conf, kvStore, live = true)
- sparkContext.listenerBus.addToStatusQueue(listener)
- val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
- sparkContext.ui.foreach(new SQLTab(statusStore, _))
- statusStore
+ // Only create SQLAppStatusListener when explicitly enabled to avoid memory overhead
+ if (conf.get(SQL_UI_APP_STATUS_LISTENER_ENABLED)) {
+ val listener = new SQLAppStatusListener(conf, kvStore, live = true)
+ sparkContext.listenerBus.addToStatusQueue(listener)
+ val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
+ sparkContext.ui.foreach(new SQLTab(statusStore, _))
+ statusStore
+ } else {
+ // When disabled, create a minimal status store without listener
+ new SQLAppStatusStore(kvStore, None)
+ }
Review Comment:
the impacts looks fine! (you may need to rebase your branch to the latest master to solve the Python failures)
So how about
1. mark the new config `spark.sql.ui.appStatusListener.enabled` as `internal`, to make it for advanced users only, as it has side effects.
2. keep the default value fallback to `spark.ui.enabled`, then disabling UI also reduces driver memory usage.
@mridulm @cloud-fan, WDYT? Does disabling `appStatusListener` have other potential impacts other than UI?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2851217570
##########
sql/core/src/main/scala/org/apache/spark/sql/internal/SharedState.scala:
##########
@@ -120,11 +120,17 @@ private[sql] class SharedState(
*/
val statusStore: SQLAppStatusStore = {
val kvStore = sparkContext.statusStore.store.asInstanceOf[ElementTrackingStore]
- val listener = new SQLAppStatusListener(conf, kvStore, live = true)
- sparkContext.listenerBus.addToStatusQueue(listener)
- val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
- sparkContext.ui.foreach(new SQLTab(statusStore, _))
- statusStore
+ // Only create SQLAppStatusListener when explicitly enabled to avoid memory overhead
+ if (conf.get(SQL_UI_APP_STATUS_LISTENER_ENABLED)) {
+ val listener = new SQLAppStatusListener(conf, kvStore, live = true)
+ sparkContext.listenerBus.addToStatusQueue(listener)
+ val statusStore = new SQLAppStatusStore(kvStore, Some(listener))
+ sparkContext.ui.foreach(new SQLTab(statusStore, _))
+ statusStore
+ } else {
+ // When disabled, create a minimal status store without listener
+ new SQLAppStatusStore(kvStore, None)
+ }
Review Comment:
the impacts looks fine! (you may need to rebase your branch to the latest master to solve the Python failures)
So how about
1. mark the new config `spark.sql.ui.appStatusListener.enabled` as `internal` for advanced users, as it has side effects.
2. keep the default value fallback to `spark.ui.enabled`, then disabling UI also reduces driver memory usage.
@mridulm @cloud-fan, WDYT? Does disabling `appStatusListener` have other potential impacts other than UI?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2851866005
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala:
##########
@@ -236,6 +236,15 @@ object StaticSQLConf {
.booleanConf
.createWithDefault(false)
+ val SQL_UI_APP_STATUS_LISTENER_ENABLED =
+ buildStaticConf("spark.sql.ui.appStatusListener.enabled")
Review Comment:
`appStatusListener` is internal, we should not expose it to users. So this is to disable SQL UI?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-55353][SQL] Add config to disable SQLAppStatusListener [spark]
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #54133:
URL: https://github.com/apache/spark/pull/54133#discussion_r2787056530
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala:
##########
@@ -236,6 +236,15 @@ object StaticSQLConf {
.booleanConf
.createWithDefault(false)
+ val SQL_UI_APP_STATUS_LISTENER_ENABLED =
+ buildStaticConf("spark.sql.ui.appStatusListener.enabled")
+ .doc("Whether to enable SQLAppStatusListener for collecting SQL execution metrics. " +
+ "Disabling this can significantly reduce driver memory usage for complex queries with " +
+ "many stages and tasks, but will result in limited SQL execution information in the UI.")
+ .version("4.0.0")
+ .booleanConf
+ .createWithDefault(true)
Review Comment:
Spark currently under 4.2 developing cycles so version should be "4.2.0".
I agree `appStatusListener` should keep enabled by default. But could you disable it and tigger a round CI, to help us evalute the impact?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org