You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "xupefei (via GitHub)" <gi...@apache.org> on 2024/11/20 11:33:43 UTC
[PR] [SPARK-50366] Isolate user-defined tags on thread level for SparkSession in Classic [spark]
xupefei opened a new pull request, #48906:
URL: https://github.com/apache/spark/pull/48906
### What changes were proposed in this pull request?
This PR changes the implementation of user-provided tags to be thread-local, so that tags added by two threads to the same SparkSession do not interfere with each other.
Overlaps (from the `SparkCcntext` perspective) are avoided by introducing a thread-local random UUID which is attached to all tags in the same thread.
### Why are the changes needed?
To make tags isolated per thread.
### Does this PR introduce _any_ user-facing change?
Yes, user-provided tags are now isolated on the session level.
### How was this patch tested?
Local test.
### Was this patch authored or co-authored using generative AI tooling?
No.
--
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-50366][SQL] Isolate user-defined tags on thread level for SparkSession in Classic [spark]
Posted by "itholic (via GitHub)" <gi...@apache.org>.
itholic commented on code in PR #48906:
URL: https://github.com/apache/spark/pull/48906#discussion_r1851594844
##########
sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala:
##########
@@ -133,14 +133,34 @@ class SparkSession private(
/** Tag to mark all jobs owned by this session. */
private[sql] lazy val sessionJobTag = s"spark-session-$sessionUUID"
+ /**
+ * A UUID that is unique on the thread level. Used by managedJobTags to make sure that a same
+ * tag from two threads does not overlap in the underlying SparkContext/SQLExecution.
+ */
+ private[sql] lazy val threadUuid = new InheritableThreadLocal[String] {
+ override def childValue(parent: String): String = parent
+
+ override def initialValue(): String = UUID.randomUUID().toString
+ }
+
/**
* A map to hold the mapping from user-defined tags to the real tags attached to Jobs.
- * Real tag have the current session ID attached: `"tag1" -> s"spark-session-$sessionUUID-tag1"`.
+ * Real tag have the current session ID attached:
+ * tag1" -> s"spark-session-$sessionUUID-thread-$threadUuid-tag1
Review Comment:
nit: quotes for string seems a bit wired?
--
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-50366][SQL] Isolate user-defined tags on thread level for SparkSession in Classic [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #48906:
URL: https://github.com/apache/spark/pull/48906#discussion_r1851125378
##########
sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala:
##########
@@ -133,14 +133,34 @@ class SparkSession private(
/** Tag to mark all jobs owned by this session. */
private[sql] lazy val sessionJobTag = s"spark-session-$sessionUUID"
+ /**
+ * A UUID that is unique on the thread level. Used by managedJobTags to make sure that a same
+ * tag from two threads does not overlap in the underlying SparkContext/SQLExecution.
+ */
+ private[sql] lazy val threadUuid = new InheritableThreadLocal[String] {
Review Comment:
Could we have a simple test for this inheritance? Otherwise, looks fine from a cursory 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-50366][SQL] Isolate user-defined tags on thread level for SparkSession in Classic [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon closed pull request #48906: [SPARK-50366][SQL] Isolate user-defined tags on thread level for SparkSession in Classic
URL: https://github.com/apache/spark/pull/48906
--
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-50366][SQL] Isolate user-defined tags on thread level for SparkSession in Classic [spark]
Posted by "xupefei (via GitHub)" <gi...@apache.org>.
xupefei commented on code in PR #48906:
URL: https://github.com/apache/spark/pull/48906#discussion_r1851764566
##########
sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala:
##########
@@ -133,14 +133,34 @@ class SparkSession private(
/** Tag to mark all jobs owned by this session. */
private[sql] lazy val sessionJobTag = s"spark-session-$sessionUUID"
+ /**
+ * A UUID that is unique on the thread level. Used by managedJobTags to make sure that a same
+ * tag from two threads does not overlap in the underlying SparkContext/SQLExecution.
+ */
+ private[sql] lazy val threadUuid = new InheritableThreadLocal[String] {
+ override def childValue(parent: String): String = parent
+
+ override def initialValue(): String = UUID.randomUUID().toString
+ }
+
/**
* A map to hold the mapping from user-defined tags to the real tags attached to Jobs.
- * Real tag have the current session ID attached: `"tag1" -> s"spark-session-$sessionUUID-tag1"`.
+ * Real tag have the current session ID attached:
+ * tag1" -> s"spark-session-$sessionUUID-thread-$threadUuid-tag1
Review Comment:
Oops, Fixed!
--
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-50366][SQL] Isolate user-defined tags on thread level for SparkSession in Classic [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #48906:
URL: https://github.com/apache/spark/pull/48906#issuecomment-2489764195
cc @itholic too
--
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-50366][SQL] Isolate user-defined tags on thread level for SparkSession in Classic [spark]
Posted by "xupefei (via GitHub)" <gi...@apache.org>.
xupefei commented on code in PR #48906:
URL: https://github.com/apache/spark/pull/48906#discussion_r1851763160
##########
sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala:
##########
@@ -133,14 +133,34 @@ class SparkSession private(
/** Tag to mark all jobs owned by this session. */
private[sql] lazy val sessionJobTag = s"spark-session-$sessionUUID"
+ /**
+ * A UUID that is unique on the thread level. Used by managedJobTags to make sure that a same
+ * tag from two threads does not overlap in the underlying SparkContext/SQLExecution.
+ */
+ private[sql] lazy val threadUuid = new InheritableThreadLocal[String] {
Review Comment:
Yes we do! See
https://github.com/apache/spark/pull/48906/files#diff-647cd00e281c3cccb76a42646a692abaef6738c8157daf6642bc8fa16e233394R174-R175
--
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-50366][SQL] Isolate user-defined tags on thread level for SparkSession in Classic [spark]
Posted by "xupefei (via GitHub)" <gi...@apache.org>.
xupefei commented on code in PR #48906:
URL: https://github.com/apache/spark/pull/48906#discussion_r1850246910
##########
sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala:
##########
@@ -133,14 +133,32 @@ class SparkSession private(
/** Tag to mark all jobs owned by this session. */
private[sql] lazy val sessionJobTag = s"spark-session-$sessionUUID"
+ /**
+ * A UUID that is unique on the thread level. Used by managedJobTags to make sure that the same
+ * use tag do not overlap in the underlying SparkContext/SQLExecution.
+ */
+ private[sql] lazy val threadUuid = new ThreadLocal[String] {
Review Comment:
Thanks, fixed!
I was confused about thread vs session. The `Inheritable` here is for child threads.
--
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-50366][SQL] Isolate user-defined tags on thread level for SparkSession in Classic [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #48906:
URL: https://github.com/apache/spark/pull/48906#discussion_r1850166257
##########
sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala:
##########
@@ -133,14 +133,32 @@ class SparkSession private(
/** Tag to mark all jobs owned by this session. */
private[sql] lazy val sessionJobTag = s"spark-session-$sessionUUID"
+ /**
+ * A UUID that is unique on the thread level. Used by managedJobTags to make sure that the same
+ * use tag do not overlap in the underlying SparkContext/SQLExecution.
+ */
+ private[sql] lazy val threadUuid = new ThreadLocal[String] {
Review Comment:
Hm, would have to be InheritableThreadLocal. I believe that's how Spark Connect works.
--
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-50366][SQL] Isolate user-defined tags on thread level for SparkSession in Classic [spark]
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #48906:
URL: https://github.com/apache/spark/pull/48906#issuecomment-2492620527
Merged to master.
--
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