[SPARK-49544][CONNECT] Replace coarse-locking in SparkConnectExecutionManager with ConcurrentMap #48034
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
changgyoopark-db
changed the title
changgyoopark-db
changed the title
|
@juliuszsompolski Can you please review this PR too? Thanks! |
...erver/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectExecutionManager.scala
Show resolved
Hide resolved
...erver/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectExecutionManager.scala
Show resolved
Hide resolved
...erver/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectExecutionManager.scala
Show resolved
Hide resolved
...erver/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectExecutionManager.scala
Show resolved
Hide resolved
changgyoopark-db
force-pushed
the
SPARK-49544
branch
from
773a6f0 to
c4d116b
Compare
juliuszsompolski
approved these changes
Sep 10, 2024
|
@hvanhovell @HyukjinKwon Hello Herman and Hyukjin, would you mind merging this PR? Thanks! |
|
@grundprinzip Hey Martin, can you review and merge this change? Thanks! |
hvanhovell
reviewed
Sep 11, 2024
...erver/src/main/scala/org/apache/spark/sql/connect/service/SparkConnectExecutionManager.scala
Outdated
Show resolved
Hide resolved
changgyoopark-db
force-pushed
the
SPARK-49544
branch
from
c4d116b to
43898ac
Compare
HyukjinKwon
pushed a commit
that referenced
this pull request
Sep 11, 2024
…anager with ConcurrentMap ### What changes were proposed in this pull request? Replace the coarse-locking in SparkConnectSessionManager with ConcurrentMap in order to minimise lock contention when there are many sessions. ### Why are the changes needed? It is a spin-off from #48034 where #48034 addresses many-execution cases whereas this addresses many-session situations. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing test cases. ### Was this patch authored or co-authored using generative AI tooling? No. Closes #48036 from changgyoopark-db/SPARK-49548. Authored-by: Changgyoo Park <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
HyukjinKwon
approved these changes
Sep 11, 2024
|
Merged to master. |
attilapiros
pushed a commit
to attilapiros/spark
that referenced
this pull request
Oct 4, 2024
…anager with ConcurrentMap ### What changes were proposed in this pull request? Replace the coarse-locking in SparkConnectSessionManager with ConcurrentMap in order to minimise lock contention when there are many sessions. ### Why are the changes needed? It is a spin-off from apache#48034 where apache#48034 addresses many-execution cases whereas this addresses many-session situations. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing test cases. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#48036 from changgyoopark-db/SPARK-49548. Authored-by: Changgyoo Park <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
attilapiros
pushed a commit
to attilapiros/spark
that referenced
this pull request
Oct 4, 2024
…nManager with ConcurrentMap ### What changes were proposed in this pull request? Replace the coarse-locking mechanism implemented in SparkConnectExecutionManager with ConcurrentMap in order to ameliorate lock contention. ### Why are the changes needed? When there are too many threads, e.g., ~10K threads on a 4-core node, OS scheduling may cause priority inversion that leads to a serious performance problems, e.g., a 1000s delay when reattaching to an execute holder. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing test cases. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#48034 from changgyoopark-db/SPARK-49544. Authored-by: Changgyoo Park <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
himadripal
pushed a commit
to himadripal/spark
that referenced
this pull request
Oct 19, 2024
…anager with ConcurrentMap ### What changes were proposed in this pull request? Replace the coarse-locking in SparkConnectSessionManager with ConcurrentMap in order to minimise lock contention when there are many sessions. ### Why are the changes needed? It is a spin-off from apache#48034 where apache#48034 addresses many-execution cases whereas this addresses many-session situations. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing test cases. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#48036 from changgyoopark-db/SPARK-49548. Authored-by: Changgyoo Park <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
himadripal
pushed a commit
to himadripal/spark
that referenced
this pull request
Oct 19, 2024
…nManager with ConcurrentMap ### What changes were proposed in this pull request? Replace the coarse-locking mechanism implemented in SparkConnectExecutionManager with ConcurrentMap in order to ameliorate lock contention. ### Why are the changes needed? When there are too many threads, e.g., ~10K threads on a 4-core node, OS scheduling may cause priority inversion that leads to a serious performance problems, e.g., a 1000s delay when reattaching to an execute holder. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Existing test cases. ### Was this patch authored or co-authored using generative AI tooling? No. Closes apache#48034 from changgyoopark-db/SPARK-49544. Authored-by: Changgyoo Park <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Replace the coarse-locking mechanism implemented in SparkConnectExecutionManager with ConcurrentMap in order to ameliorate lock contention.
Why are the changes needed?
When there are too many threads, e.g., ~10K threads on a 4-core node, OS scheduling may cause priority inversion that leads to a serious performance problems, e.g., a 1000s delay when reattaching to an execute holder.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing test cases.
Was this patch authored or co-authored using generative AI tooling?
No.