You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/01/17 04:48:41 UTC
[GitHub] [spark] vinooganesh opened a new pull request #35224: [SPARK-32165][SQL] Ensure Spark only initiates SharedState once across SparkSessions
vinooganesh opened a new pull request #35224:
URL: https://github.com/apache/spark/pull/35224
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
The memory leak that was partially fixed in https://github.com/apache/spark/pull/28128 doesn't cover the case where sessionState is touched. From the initial description: "Once SessionState is touched, it will add two more listeners into the SparkContext, namely SQLAppStatusListener and ExecutionListenerBus."
### Why are the changes needed?
This fixes a memory leak that that can cause a spark application to oom if many spark sessions are created
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
Unit test included as a part of the PR.
--
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
[GitHub] [spark] cloud-fan commented on pull request #35224: [SPARK-32165][SQL] Ensure Spark only initiates SharedState once across SparkSessions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35224:
URL: https://github.com/apache/spark/pull/35224#issuecomment-1014575419
Then can you explain more about how the memory leak happens? such as the object references path and the GC root? The added listeners can be GCed after https://github.com/apache/spark/commit/4d90c5dc0efcf77ef6735000ee7016428c57077b so I don't see how it can cause memory leak again.
It will be great if you can share the heap dump when the memory leak happens.
--
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
[GitHub] [spark] dnskr commented on pull request #35224: [SPARK-32165][SQL] Ensure Spark only initiates SharedState once across SparkSessions
Posted by GitBox <gi...@apache.org>.
dnskr commented on pull request #35224:
URL: https://github.com/apache/spark/pull/35224#issuecomment-1014968891
I haven't seen https://github.com/apache/spark/commit/4d90c5dc0efcf77ef6735000ee7016428c57077b either. The change fixes `ExecutionListenerBus` memory leak but not the memory leak of `SQLAppStatusListener` in case when new `SessionState` is touched.
I have created similar [PR#35234](https://github.com/apache/spark/pull/35234). @cloud-fan @vinooganesh could you please have a look into it?
Regarding heap dump and related things, I don't have good experience with memory leak investigations so would highly appreciate if you could help with it.
--
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
[GitHub] [spark] vinooganesh commented on pull request #35224: [SPARK-32165][SQL] Ensure Spark only initiates SharedState once across SparkSessions
Posted by GitBox <gi...@apache.org>.
vinooganesh commented on pull request #35224:
URL: https://github.com/apache/spark/pull/35224#issuecomment-1014140620
Hey @cloud-fan - could I please get an "ok to test" here (and any thoughts you have about this approach)? Seems like this issue is hitting folks in production per comments on this: https://issues.apache.org/jira/browse/SPARK-32165.
--
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
[GitHub] [spark] dnskr commented on pull request #35224: [SPARK-32165][SQL] Ensure Spark only initiates SharedState once across SparkSessions
Posted by GitBox <gi...@apache.org>.
dnskr commented on pull request #35224:
URL: https://github.com/apache/spark/pull/35224#issuecomment-1014482302
@cloud-fan It is old memory leak and originally it was mentioned in the [PR#28128](https://github.com/apache/spark/pull/28128) and [comment](https://github.com/apache/spark/pull/28128#issuecomment-653102770).
I reproduce the issue on Spark 3.2.0 by running the code from the comment and I see the same result:
`SQLAppStatusListener` and `ExecutionListenerBus` added each time called
```
SparkSession
.builder()
.sparkContext(context)
.master("local")
.getOrCreate()
.sessionState // this touches the sessionState
```
--
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
[GitHub] [spark] vinooganesh commented on pull request #35224: [SPARK-32165][SQL] Ensure Spark only initiates SharedState once across SparkSessions
Posted by GitBox <gi...@apache.org>.
vinooganesh commented on pull request #35224:
URL: https://github.com/apache/spark/pull/35224#issuecomment-1014802764
@dnskr - I will defer to you here to provide this information given that it sounds like you are hitting this in prod and are experiencing production failures (I'm mostly operating off of the info from your comment on the jira ticket). I hadn't previously seen https://github.com/apache/spark/commit/4d90c5dc0efcf77ef6735000ee7016428c57077b, and it does look like that should resolve this issue, but if we have a case to the contrary in prod, we can definitely investigate further.
If it's just the unit test from that other ticket that's failing, that's likely fine given that after https://github.com/apache/spark/commit/4d90c5dc0efcf77ef6735000ee7016428c57077b, the orphaned listeners will just be GCed away. This test replicates this behavior: https://github.com/apache/spark/blob/661b80da8aa7a6b82f6cb07cc7c72de6a2504426/sql/core/src/test/scala/org/apache/spark/sql/SparkSessionBuilderSuite.scala#L49
--
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
[GitHub] [spark] vinooganesh commented on pull request #35224: [SPARK-32165][SQL] Ensure Spark only initiates SharedState once across SparkSessions
Posted by GitBox <gi...@apache.org>.
vinooganesh commented on pull request #35224:
URL: https://github.com/apache/spark/pull/35224#issuecomment-1016644297
Closing per comment in https://issues.apache.org/jira/browse/SPARK-32165. Thanks @cloud-fan !
--
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
[GitHub] [spark] cloud-fan commented on pull request #35224: [SPARK-32165][SQL] Ensure Spark only initiates SharedState once across SparkSessions
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35224:
URL: https://github.com/apache/spark/pull/35224#issuecomment-1014213661
I thought the memory lead has been fixed by https://github.com/apache/spark/commit/4d90c5dc0efcf77ef6735000ee7016428c57077b ?
If there is a new memory leak, can you explain the object reference path from the GC root?
--
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
[GitHub] [spark] AmplabJenkins commented on pull request #35224: [SPARK-32165][SQL] Ensure Spark only initiates SharedState once across SparkSessions
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #35224:
URL: https://github.com/apache/spark/pull/35224#issuecomment-1014440333
Can one of the admins verify this patch?
--
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
[GitHub] [spark] vinooganesh closed pull request #35224: [SPARK-32165][SQL] Ensure Spark only initiates SharedState once across SparkSessions
Posted by GitBox <gi...@apache.org>.
vinooganesh closed pull request #35224:
URL: https://github.com/apache/spark/pull/35224
--
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
[GitHub] [spark] dnskr commented on pull request #35224: [SPARK-32165][SQL] Ensure Spark only initiates SharedState once across SparkSessions
Posted by GitBox <gi...@apache.org>.
dnskr commented on pull request #35224:
URL: https://github.com/apache/spark/pull/35224#issuecomment-1016630939
@vinooganesh Related jira ticket https://issues.apache.org/jira/browse/SPARK-32165 has been closed so you can close the PR.
--
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