Skip to content

[SPARK-30556][SQL][FOLLOWUP] Reset the status changed in SQLExecution.withThreadLocalCaptured - #27516

Closed
xuanyuanking wants to merge 3 commits into
apache:masterfrom
xuanyuanking:SPARK-30556-follow
Closed

[SPARK-30556][SQL][FOLLOWUP] Reset the status changed in SQLExecution.withThreadLocalCaptured#27516
xuanyuanking wants to merge 3 commits into
apache:masterfrom
xuanyuanking:SPARK-30556-follow

Conversation

@xuanyuanking

@xuanyuankingxuanyuanking commented Feb 10, 2020

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Follow up for #27267, reset the status changed in SQLExecution.withThreadLocalCaptured.

Why are the changes needed?

For code safety.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing UT.

@xuanyuanking

xuanyuanking commented Feb 10, 2020

Copy link
Copy Markdown
MemberAuthor

cc @ajithme@cloud-fan@hvanhovell@gatorsmile

cloud-fan
cloud-fan reviewed Feb 10, 2020
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/execution/SQLExecution.scala Outdated
cloud-fan
cloud-fan reviewed Feb 10, 2020
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/execution/SQLExecution.scala Outdated
val localProps = Utils.cloneProperties(sc.getLocalProperties)
Future {
val originalContext = SparkSession.getActiveSession
val originalLocalProps = Utils.cloneProperties(sc.getLocalProperties)

@cloud-fancloud-fanFeb 10, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to clone?

@xuanyuankingxuanyuankingFeb 10, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can assure the LocalProps is used in current thread here, getLocalProperties is enough, done in a33154c, thanks.

@ajithmeajithmeFeb 10, 2020•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think clone is required. Please read
#27267 (comment)

Also i dont see why we need to reset. By default pool threads do not have any localproperties or localsession variable set. That means after the thread is used for first time, this change will always try to retain those properties and session by always finally resetting it to same, which is kind of stale already. Right.?

@xuanyuankingxuanyuankingFeb 10, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think clone is required.

The two scenarios are different, the first one needs clone since we want a snapshot of local properties during submit. But for this one, we need to reset by the variable self. If use clone here, changes in other places might be dropped.

Also i dont see why we need to reset.

For the function added in SQLExecution here, I think we need to clean up all the side effects as far as possible, even though it works in this case.

@SparkQA

SparkQA commented Feb 10, 2020

Copy link
Copy Markdown

Test build #118115 has finished for PR 27516 at commit 0128a37.

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

cloud-fan
cloud-fan reviewed Feb 10, 2020
View reviewed changes
Comment threadsql/core/src/main/scala/org/apache/spark/sql/execution/SQLExecution.scala
@SparkQA

SparkQA commented Feb 10, 2020

Copy link
Copy Markdown

Test build #118119 has finished for PR 27516 at commit a33154c.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

HyukjinKwon
HyukjinKwon reviewed Feb 10, 2020
View reviewed changes
Comment threadsql/core/src/test/scala/org/apache/spark/sql/internal/ExecutorSideSQLConfSuite.scala

// set local configuration and assert
val confValue1 = "e"
val confValue1 = UUID.randomUUID().toString()

@HyukjinKwonHyukjinKwonFeb 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is particularly needed. The value here doesn't matter at all. It doesn't test if the given value itself was correct or not. It just tests if the value is set or not. But .. I am okay.

@SparkQA

SparkQA commented Feb 10, 2020

Copy link
Copy Markdown

Test build #118142 has finished for PR 27516 at commit 51f7fc8.

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

@cloud-fan

cloud-fan commented Feb 10, 2020

Copy link
Copy Markdown
Contributor

retest this please

@SparkQA

SparkQA commented Feb 10, 2020

Copy link
Copy Markdown

Test build #118160 has finished for PR 27516 at commit 51f7fc8.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

hvanhovell
hvanhovell approved these changes Feb 10, 2020
View reviewed changes

@hvanhovellhvanhovell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks for addressing this!

@hvanhovell

hvanhovell commented Feb 10, 2020

Copy link
Copy Markdown
Contributor

Merging in master/3.0.

hvanhovell pushed a commit that referenced this pull request Feb 10, 2020
@xuanyuanking@hvanhovell
[SPARK-30556][SQL][FOLLOWUP] Reset the status changed in SQLExecution…
3038a81
….withThreadLocalCaptured
### What changes were proposed in this pull request?
Follow up for #27267, reset the status changed in SQLExecution.withThreadLocalCaptured.
### Why are the changes needed?
For code safety.
### Does this PR introduce any user-facing change?
No.
### How was this patch tested?
Existing UT.
Closes#27516 from xuanyuanking/SPARK-30556-follow.
Authored-by: Yuanjian Li <xyliyuanjian@gmail.com>
Signed-off-by: herman <herman@databricks.com>
(cherry picked from commit a6b91d2)
Signed-off-by: herman <herman@databricks.com>
@xuanyuanking

xuanyuanking commented Feb 11, 2020

Copy link
Copy Markdown
MemberAuthor

Thanks all for review.

@xuanyuanking
xuanyuanking deleted the SPARK-30556-follow branch February 11, 2020 02:36
@gatorsmile

gatorsmile commented Feb 19, 2020

Copy link
Copy Markdown
Member

The original PR #27340 was merged to 2.4 too. Should we fix 2.4?

@xuanyuanking

xuanyuanking commented Feb 19, 2020

Copy link
Copy Markdown
MemberAuthor

Copy, will send a backport.

xuanyuanking added a commit to xuanyuanking/spark that referenced this pull request Feb 19, 2020
@xuanyuanking
[SPARK-30556][SQL][FOLLOWUP] Reset the status changed in SQLExecution…
936c5b3
….withThreadLocalCaptured
Follow up for apache#27267, reset the status changed in SQLExecution.withThreadLocalCaptured.
For code safety.
No.
Existing UT.
Closesapache#27516 from xuanyuanking/SPARK-30556-follow.
(cherry picked from commit a6b91d2)
sjincho pushed a commit to sjincho/spark that referenced this pull request Apr 15, 2020
@xuanyuankingSeongjin Cho
[SPARK-30556][SQL][FOLLOWUP] Reset the status changed in SQLExecution…
5f37593
….withThreadLocalCaptured
### What changes were proposed in this pull request?
Follow up for apache#27267, reset the status changed in SQLExecution.withThreadLocalCaptured.
### Why are the changes needed?
For code safety.
### Does this PR introduce any user-facing change?
No.
### How was this patch tested?
Existing UT.
Closesapache#27516 from xuanyuanking/SPARK-30556-follow.
Authored-by: Yuanjian Li <xyliyuanjian@gmail.com>
Signed-off-by: herman <herman@databricks.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fancloud-fancloud-fan approved these changes
@HyukjinKwonHyukjinKwonHyukjinKwon approved these changes
@hvanhovellhvanhovellhvanhovell approved these changes
+1 more reviewer
@ajithmeajithmeajithme left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@xuanyuanking@SparkQA@cloud-fan@hvanhovell@gatorsmile@HyukjinKwon@ajithme@dongjoon-hyun