[SPARK-32881][CORE] Catch some race condition errors and log them more clearly by holdenk · Pull Request #29992 · apache/spark

holdenk · 2020-10-09T21:07:04Z

What changes were proposed in this pull request?

Decommissioning can run out of time resulting in some race condition, these race conditions result in confusing error messages but not negative impact.

Why are the changes needed?

The NPE & element missing errors in the log can create a missunderstanding.

Does this PR introduce any user-facing change?

Logs change.

How was this patch tested?

Existing tests pass.

…to worry users

SparkQA · 2020-10-09T21:52:07Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34203/

SparkQA · 2020-10-09T22:19:36Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34203/

SparkQA · 2020-10-09T23:42:11Z

Test build #129599 has finished for PR 29992 at commit 1ca46a4.

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

dongjoon-hyun · 2020-10-10T03:59:33Z

One K8s IT fails. Let's retrigger.

- Run in client mode. *** FAILED ***

dongjoon-hyun · 2020-10-10T03:59:41Z

Retest this please

dongjoon-hyun · 2020-10-10T04:01:40Z

core/src/main/scala/org/apache/spark/MapOutputTracker.scala

+          logWarning(s"Asked to update map output ${mapId} for untracked map status.")
+      }
+    } catch {
+      case e: java.lang.NullPointerException =>


Shall we remove java.lang.?

dongjoon-hyun · 2020-10-10T04:03:37Z

core/src/main/scala/org/apache/spark/storage/BlockManagerMasterEndpoint.scala

+      }.toSeq
+    } catch {
+      // If the block manager has already exited, nothing to replicate.
+      case e: java.util.NoSuchElementException =>


Shall we import?

Also, it would be great if we have a warning log here.

dongjoon-hyun

Thank you, @holdenk ! It looks helpful. I left a few comments.

SparkQA · 2020-10-10T04:43:52Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34215/

SparkQA · 2020-10-10T05:09:08Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34215/

SparkQA · 2020-10-10T07:05:01Z

Test build #129612 has finished for PR 29992 at commit 1ca46a4.

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

dongjoon-hyun

+1, LGTM. Thank you, @holdenk .
Merged to master.

…e clearly ### What changes were proposed in this pull request? Decommissioning can run out of time resulting in some race condition, these race conditions result in confusing error messages but not negative impact. ### Why are the changes needed? The NPE & element missing errors in the log can create a missunderstanding. ### Does this PR introduce _any_ user-facing change? Logs change. ### How was this patch tested? Existing tests pass. Closes apache#29992 from holdenk/SPARK-32881-error-messaging-on-decom-race-messages. Authored-by: Holden Karau <hkarau@apple.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

HyukjinKwon · 2020-10-14T02:09:54Z

core/src/main/scala/org/apache/spark/MapOutputTracker.scala

+          logWarning(s"Asked to update map output ${mapId} for untracked map status.")
+      }
+    } catch {
+      case e: java.lang.NullPointerException =>


Quick question: can we avoid catching NullPointerException? It's a bit odd that we catch NullPointerException. We could just switch to if-else I guess.

HyukjinKwon · 2020-10-14T02:10:44Z

How did we test this, @holdenk? I can make a quick followup for that.

HyukjinKwon · 2020-10-16T04:58:38Z

gentle ping. This PR introduces the very first place that catches and suppresses NullPointerException in the codebase - the second place that catches NullPointerException (mllib/src/main/scala/org/apache/spark/ml/feature/VectorAssembler.scala) rethrows NPE.

HyukjinKwon · 2020-10-21T00:27:56Z

gentile ping. Would you mind asking to address my comments? I thought it wouldn't be difficult to address.

holdenk · 2020-10-21T00:55:44Z

Hey sorry I didn’t see your comments. To avoid this I don’t think we could do if else, we’d have to introduce locking I think. Since this only happens under a race condition where we don’t care about the data I think it’s ok. If you’d prefer we could also take that part out since the NPE will get eaten later. Happy to review a PR with whatever approach you want or work on a follow up if you want.

HyukjinKwon · 2020-10-21T01:05:59Z

I can give a try. Do you have any stacktrace or symptom for this NPE issue? Seems SPARK-32881 did not throw NPE.

@dongjoon-hyun, should SPARK-32881 be resolved after this fix, or is it open mistakenly?

dongjoon-hyun · 2020-10-21T06:14:46Z

Thanks, @HyukjinKwon . I resolved SPARK-32881 now.

dongjoon-hyun · 2020-10-21T06:23:23Z

Actually, this PR fixes both java.lang.NullPointerException and java.util.NoSuchElementException.
I reported NPE to her personally in addition to SPARK-32881.

HyukjinKwon · 2020-10-21T06:40:23Z

That's good. I just wonder if we can just:

blockManagerInfo.contains(...) instead of catching java.util.NoSuchElementException
Check if something is null, and just ignore instead of catching java.lang.NullPointerException

I don't believe catching java.lang.NullPointerException in a block is a good practice because it will catch all NPE that can potentially happen everywhere.

Would you mind if I ask the stacktrace? At least I want to understand why catching NPE is necessary even if I fail to make a fix.

holdenk · 2020-10-21T15:07:11Z

So I think we get the null if the mapStatus is deleted during the update. The problem is checking if it's null doesn't guarantee it won't be deleted after the check.

…e clearly ### What changes were proposed in this pull request? Decommissioning can run out of time resulting in some race condition, these race conditions result in confusing error messages but not negative impact. ### Why are the changes needed? The NPE & element missing errors in the log can create a missunderstanding. ### Does this PR introduce _any_ user-facing change? Logs change. ### How was this patch tested? Existing tests pass. Closes apache#29992 from holdenk/SPARK-32881-error-messaging-on-decom-race-messages. Authored-by: Holden Karau <hkarau@apple.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

HyukjinKwon · 2020-10-28T06:28:39Z

@holdenk, I am very sorry but can you guide me how you tested and/or share a traceback?

Catch some race condition errors and log them more clearly so as not …

1ca46a4

…to worry users

dongjoon-hyun reviewed Oct 10, 2020

View reviewed changes

dongjoon-hyun approved these changes Oct 10, 2020

View reviewed changes

dongjoon-hyun closed this in 7696ca5 Oct 10, 2020

HyukjinKwon reviewed Oct 14, 2020

View reviewed changes

Conversation

holdenk commented Oct 9, 2020

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 Oct 9, 2020

Uh oh!

SparkQA commented Oct 9, 2020

Uh oh!

SparkQA commented Oct 9, 2020

Uh oh!

dongjoon-hyun commented Oct 10, 2020

Uh oh!

dongjoon-hyun commented Oct 10, 2020

Uh oh!

dongjoon-hyun Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun Oct 10, 2020

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 10, 2020

Uh oh!

SparkQA commented Oct 10, 2020

Uh oh!

SparkQA commented Oct 10, 2020

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

HyukjinKwon commented Oct 14, 2020

Uh oh!

HyukjinKwon commented Oct 16, 2020

Uh oh!

HyukjinKwon commented Oct 21, 2020

Uh oh!

holdenk commented Oct 21, 2020

Uh oh!

HyukjinKwon commented Oct 21, 2020

Uh oh!

dongjoon-hyun commented Oct 21, 2020

Uh oh!

dongjoon-hyun commented Oct 21, 2020

Uh oh!

HyukjinKwon commented Oct 21, 2020

Uh oh!

holdenk commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HyukjinKwon commented Oct 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

holdenk commented Oct 21, 2020 •

edited

Loading