[SPARK-29046][SQL] Fix NPE in SQLConf.get when active SparkContext is stopping #25753

HeartSaVioR · 2019-09-11T07:59:50Z

What changes were proposed in this pull request?

This patch fixes the bug regarding NPE in SQLConf.get, which is only possible when SparkContext._dagScheduler is null due to stopping SparkContext. The logic doesn't seem to consider active SparkContext could be in progress of stopping.

Note that it can't be encountered easily as SparkContext.stop() blocks the main thread, but there're many cases which SQLConf.get is accessed concurrently while SparkContext.stop() is executing - users run another threads, or listener is accessing SQLConf.get after dagScheduler is set to null (this is the case what I encountered.)

Why are the changes needed?

The bug brings NPE.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Added new UT to verify NPE doesn't occur. Without patch, the test fails with throwing NPE.

…ping

SparkQA · 2019-09-11T11:47:18Z

Test build #110468 has finished for PR 25753 at commit 4266ee0.

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

vanzin · 2019-09-11T17:54:37Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

    } else {
      val isSchedulerEventLoopThread = SparkContext.getActive
-        .map(_.dagScheduler.eventProcessLoop.eventThread)
+        .flatMap(sc => Option(sc.dagScheduler))


nit: we use { sc => ... } for closures. Also later.

SparkQA · 2019-09-11T22:06:50Z

Test build #110485 has finished for PR 25753 at commit a1472e8.

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

HeartSaVioR · 2019-09-11T22:11:12Z

retest this, please

SparkQA · 2019-09-12T01:53:35Z

Test build #110489 has finished for PR 25753 at commit a1472e8.

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

HyukjinKwon · 2019-09-12T02:16:16Z

Merged to master.

HeartSaVioR · 2019-09-12T04:27:32Z

Thanks all for reviewing and merging!

dongjoon-hyun · 2019-09-13T23:37:07Z

sql/core/src/test/scala/org/apache/spark/sql/internal/SQLConfSuite.scala

+        SQLConf.get
+      }
+    } {
+      oldSparkContext.orElse(Some(null)).foreach(SparkContext.setActiveContext)


Hi, All.

Ur, I'm wondering if this PR's test case is safe in concurrent testing environments?
This might be the root cause of the current outage in Apache Spark Jenkins jobs.
After this PR, all Jenkins on master branch never succeeds.

https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/

Could you take a look a little bit?

Given this is late Friday, just revert this if you thinks it's breaking things.

Yes. I've been investigating this and the other commits. But, this is the usual suspect given the error message. Since master branch has been broken over 2 days and it seems that we cannot fix this during weekend, I'll revert this.

Thanks for finding. I haven't noticed concurrent tests are executed, as there're many tests leveraging single object like SparkContext. I'll just exclude test code in new PR as I don't see other way to test this.

Btw, do we run different options between PR and master build? I'm curious about the reason - as we tend to guess PR build pass as "OK".

I think the issue is probably Maven vs SBT testing differences. I personally would prefer to standardize on the Maven build for everything.

@HeartSaVioR . PRBuilder is the same. PRBuilder has been very flaky last two days. I hit the same failure on my independent PRs too many times. That was the reason I investigated this and reached here to ask help.

@srowen . According to the Jenkins status, All Jenkins job combinations failed. This is irrelevant to SBT and Maven difference.

https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/

dongjoon-hyun · 2019-09-14T03:47:45Z

Hi, All.
Since Jenkins will be reset at midnight, I'll revert this after Jenkins reset around Tomorrow 00:15AM. Please let me know if you think you can recover Jenkins with another method.

dongjoon-hyun · 2019-09-14T07:16:44Z

FYI, this is reverted via 13b77e5 .

dongjoon-hyun · 2019-09-14T13:02:52Z

After reverting, Jenkins jobs are recovered. Please make another PR for this.

HyukjinKwon · 2019-09-15T02:58:56Z

Thanks all for taking care of this.

… stopping # What changes were proposed in this pull request? This patch fixes the bug regarding NPE in SQLConf.get, which is only possible when SparkContext._dagScheduler is null due to stopping SparkContext. The logic doesn't seem to consider active SparkContext could be in progress of stopping. Note that it can't be encountered easily as `SparkContext.stop()` blocks the main thread, but there're many cases which SQLConf.get is accessed concurrently while SparkContext.stop() is executing - users run another threads, or listener is accessing SQLConf.get after dagScheduler is set to null (this is the case what I encountered.) ### Why are the changes needed? The bug brings NPE. ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Added new UT to verify NPE doesn't occur. Without patch, the test fails with throwing NPE. Closes apache#25753 from HeartSaVioR/SPARK-29046. Authored-by: Jungtaek Lim (HeartSaVioR) <[email protected]> Signed-off-by: HyukjinKwon <[email protected]>

… stopping ### What changes were proposed in this pull request? This patch fixes the bug regarding NPE in SQLConf.get, which is only possible when SparkContext._dagScheduler is null due to stopping SparkContext. The logic doesn't seem to consider active SparkContext could be in progress of stopping. Note that it can't be encountered easily as SparkContext.stop() blocks the main thread, but there're many cases which SQLConf.get is accessed concurrently while SparkContext.stop() is executing - users run another threads, or listener is accessing SQLConf.get after dagScheduler is set to null (this is the case what I encountered.) ### Why are the changes needed? The bug brings NPE. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Previous patch #25753 was tested with new UT, and due to disruption with other tests in concurrent test run, the test is excluded in this patch. Closes #25790 from HeartSaVioR/SPARK-29046-v2. Authored-by: Jungtaek Lim (HeartSaVioR) <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

… stopping ### What changes were proposed in this pull request? This patch fixes the bug regarding NPE in SQLConf.get, which is only possible when SparkContext._dagScheduler is null due to stopping SparkContext. The logic doesn't seem to consider active SparkContext could be in progress of stopping. Note that it can't be encountered easily as SparkContext.stop() blocks the main thread, but there're many cases which SQLConf.get is accessed concurrently while SparkContext.stop() is executing - users run another threads, or listener is accessing SQLConf.get after dagScheduler is set to null (this is the case what I encountered.) ### Why are the changes needed? The bug brings NPE. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Previous patch apache#25753 was tested with new UT, and due to disruption with other tests in concurrent test run, the test is excluded in this patch. Closes apache#25790 from HeartSaVioR/SPARK-29046-v2. Authored-by: Jungtaek Lim (HeartSaVioR) <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…xt is stopping ### What changes were proposed in this pull request? This patch fixes the bug regarding NPE in SQLConf.get, which is only possible when SparkContext._dagScheduler is null due to stopping SparkContext. The logic doesn't seem to consider active SparkContext could be in progress of stopping. Note that it can't be encountered easily as SparkContext.stop() blocks the main thread, but there're many cases which SQLConf.get is accessed concurrently while SparkContext.stop() is executing - users run another threads, or listener is accessing SQLConf.get after dagScheduler is set to null (this is the case what I encountered.) ### Why are the changes needed? The bug brings NPE. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Previous patch #25753 was tested with new UT, and due to disruption with other tests in concurrent test run, the test is excluded in this patch. Closes #25798 from HeartSaVioR/SPARK-29046-branch-2.4. Authored-by: Jungtaek Lim (HeartSaVioR) <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

[SPARK-29046] Fix NPE in SQLConf.get when active SparkContext is stop…

4266ee0

…ping

HeartSaVioR changed the title ~~[SPARK-29046] Fix NPE in SQLConf.get when active SparkContext is stopping~~ [SPARK-29046][SQL] Fix NPE in SQLConf.get when active SparkContext is stopping Sep 11, 2019

srowen approved these changes Sep 11, 2019

View reviewed changes

vanzin reviewed Sep 11, 2019

View reviewed changes

Reflect review comment

a1472e8

dongjoon-hyun added the SQL label Sep 11, 2019

HyukjinKwon approved these changes Sep 12, 2019

View reviewed changes

HyukjinKwon closed this in 850833f Sep 12, 2019

dongjoon-hyun reviewed Sep 13, 2019

View reviewed changes

HeartSaVioR mentioned this pull request Sep 14, 2019

[SPARK-29046][SQL] Fix NPE in SQLConf.get when active SparkContext is stopping #25790

Closed

HeartSaVioR mentioned this pull request Sep 16, 2019

[SPARK-29046][SQL][2.4] Fix NPE in SQLConf.get when active SparkContext is stopping #25798

Closed

[SPARK-29046][SQL] Fix NPE in SQLConf.get when active SparkContext is stopping #25753

[SPARK-29046][SQL] Fix NPE in SQLConf.get when active SparkContext is stopping #25753

Uh oh!

Conversation

HeartSaVioR commented Sep 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

SparkQA commented Sep 11, 2019

Uh oh!

vanzin Sep 11, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 11, 2019

Uh oh!

HeartSaVioR commented Sep 11, 2019

Uh oh!

SparkQA commented Sep 12, 2019

Uh oh!

HyukjinKwon commented Sep 12, 2019

Uh oh!

HeartSaVioR commented Sep 12, 2019

Uh oh!

dongjoon-hyun Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanzin Sep 13, 2019

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Sep 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR Sep 14, 2019

Choose a reason for hiding this comment

Uh oh!

srowen Sep 14, 2019

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Sep 14, 2019

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Sep 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongjoon-hyun commented Sep 14, 2019

Uh oh!

dongjoon-hyun commented Sep 14, 2019

Uh oh!

HyukjinKwon commented Sep 15, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

HeartSaVioR commented Sep 11, 2019 •

edited

Loading

dongjoon-hyun Sep 13, 2019 •

edited

Loading

dongjoon-hyun Sep 14, 2019 •

edited

Loading

dongjoon-hyun commented Sep 14, 2019 •

edited

Loading