[SPARK-31941][CORE] Replace SparkException to NoSuchElementException for applicationInfo in AppStatusStore #28768

SaurabhChawla100 · 2020-06-09T09:48:34Z

What changes were proposed in this pull request?

After SPARK-31632 SparkException is thrown from def applicationInfo
def applicationInfo(): v1.ApplicationInfo = { try { // The ApplicationInfo may not be available when Spark is starting up. store.view(classOf[ApplicationInfoWrapper]).max(1).iterator().next().info } catch { case _: NoSuchElementException => throw new SparkException("Failed to get the application information. " + "If you are starting up Spark, please wait a while until it's ready.") } }

Where as the caller for this method def getSparkUser in Spark UI is not handling SparkException in the catch

def getSparkUser: String = { try { Option(store.applicationInfo().attempts.head.sparkUser) .orElse(store.environmentInfo().systemProperties.toMap.get("user.name")) .getOrElse("<unknown>") } catch { case _: NoSuchElementException => "<unknown>" } }

So On using this method (getSparkUser )we can get the application erred out.

As the part of this PR we will replace SparkException to NoSuchElementException for applicationInfo in AppStatusStore

Why are the changes needed?

On invoking the method getSparkUser, we can get the SparkException on calling store.applicationInfo(). And this is not handled in the catch block and getSparkUser will error out in this scenario

Does this PR introduce any user-facing change?

No

How was this patch tested?

Done the manual testing using the spark-shell and spark-submit

SaurabhChawla100 · 2020-06-09T09:59:03Z

core/src/main/scala/org/apache/spark/ui/SparkUI.scala

        .getOrElse("<unknown>")
    } catch {
+      case sparkException: SparkException => "<unknown>"
      case _: NoSuchElementException => "<unknown>"


this is needed since store.environmentInfo() , can throw NoSuchElementException

cc: @sarutak

nit case _: SparkException => "<unknown>"

SaurabhChawla100 · 2020-06-09T10:00:39Z

cc @HeartSaVioR, @srowen @HyukjinKwon @dongjoon-hyun

Please review this PR

maropu · 2020-06-09T10:38:42Z

ok to test

…on and it can handle this

SparkQA · 2020-06-09T14:05:23Z

Test build #123690 has finished for PR 28768 at commit 2b55c24.

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

SparkQA · 2020-06-09T14:40:19Z

Test build #123691 has finished for PR 28768 at commit b75d168.

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

SparkQA · 2020-06-09T14:45:35Z

Test build #123692 has finished for PR 28768 at commit 99584f5.

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

sarutak · 2020-06-09T15:50:50Z

core/src/main/scala/org/apache/spark/ui/SparkUI.scala

        .getOrElse("<unknown>")
    } catch {
-      case _: NoSuchElementException => "<unknown>"
+      case _: SparkException => "<unknown>"


How about replacing SparkException with NoSuchElementException here?
That method threw NoSuchElementException before #28444 so I think it's better to keep the type of exception.
@HyukjinKwon suggested NoSuchElementException here but finally SparkException is used somehow.

I've tried replacing SparkException with NoSuchElementException as I suggested and I can get the same error message as the screenshot shown here except the type of exception.

Sure , I have replaced SparkException with NoSuchElementException. Please validate

SparkQA · 2020-06-09T19:04:36Z

Test build #123700 has finished for PR 28768 at commit e8769fb.

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

HeartSaVioR · 2020-06-10T00:12:33Z

The change looks good - we may need to reflect the "actual change" into PR title and description, as it's no longer same as initial proposal.

SaurabhChawla100 · 2020-06-10T03:32:19Z

The change looks good - we may need to reflect the "actual change" into PR title and description, as it's no longer same as initial proposal.

I have updated the PR title and description in this

HeartSaVioR

LGTM

Ngone51 · 2020-06-10T04:05:59Z

cc @xccui @jiangxb1987

xccui · 2020-06-10T04:33:06Z

Sorry that I didn't realize the potential impact of using SparkException or NoSuchElementException.

+1 to this change.

jiangxb1987

LGTM

sarutak · 2020-06-10T07:48:23Z

Thanks all, merging into master, branch-3.0 and branch-2.4.

…for applicationInfo in AppStatusStore ### What changes were proposed in this pull request? After SPARK-31632 SparkException is thrown from def applicationInfo `def applicationInfo(): v1.ApplicationInfo = { try { // The ApplicationInfo may not be available when Spark is starting up. store.view(classOf[ApplicationInfoWrapper]).max(1).iterator().next().info } catch { case _: NoSuchElementException => throw new SparkException("Failed to get the application information. " + "If you are starting up Spark, please wait a while until it's ready.") } }` Where as the caller for this method def getSparkUser in Spark UI is not handling SparkException in the catch `def getSparkUser: String = { try { Option(store.applicationInfo().attempts.head.sparkUser) .orElse(store.environmentInfo().systemProperties.toMap.get("user.name")) .getOrElse("<unknown>") } catch { case _: NoSuchElementException => "<unknown>" } }` So On using this method (getSparkUser )we can get the application erred out. As the part of this PR we will replace SparkException to NoSuchElementException for applicationInfo in AppStatusStore ### Why are the changes needed? On invoking the method getSparkUser, we can get the SparkException on calling store.applicationInfo(). And this is not handled in the catch block and getSparkUser will error out in this scenario ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Done the manual testing using the spark-shell and spark-submit Closes #28768 from SaurabhChawla100/SPARK-31941. Authored-by: SaurabhChawla <[email protected]> Signed-off-by: Kousuke Saruta <[email protected]> (cherry picked from commit 82ff29b) Signed-off-by: Kousuke Saruta <[email protected]>

…for applicationInfo in AppStatusStore ### What changes were proposed in this pull request? After SPARK-31632 SparkException is thrown from def applicationInfo `def applicationInfo(): v1.ApplicationInfo = { try { // The ApplicationInfo may not be available when Spark is starting up. store.view(classOf[ApplicationInfoWrapper]).max(1).iterator().next().info } catch { case _: NoSuchElementException => throw new SparkException("Failed to get the application information. " + "If you are starting up Spark, please wait a while until it's ready.") } }` Where as the caller for this method def getSparkUser in Spark UI is not handling SparkException in the catch `def getSparkUser: String = { try { Option(store.applicationInfo().attempts.head.sparkUser) .orElse(store.environmentInfo().systemProperties.toMap.get("user.name")) .getOrElse("<unknown>") } catch { case _: NoSuchElementException => "<unknown>" } }` So On using this method (getSparkUser )we can get the application erred out. As the part of this PR we will replace SparkException to NoSuchElementException for applicationInfo in AppStatusStore ### Why are the changes needed? On invoking the method getSparkUser, we can get the SparkException on calling store.applicationInfo(). And this is not handled in the catch block and getSparkUser will error out in this scenario ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Done the manual testing using the spark-shell and spark-submit Closes apache#28768 from SaurabhChawla100/SPARK-31941. Authored-by: SaurabhChawla <[email protected]> Signed-off-by: Kousuke Saruta <[email protected]> (cherry picked from commit 82ff29b) Signed-off-by: Kousuke Saruta <[email protected]>

Add the code change to handle the SparkException

6b2d33c

probot-autolabeler bot added CORE WEB UI labels Jun 9, 2020

Add the code change to handle the SparkException

2b55c24

SaurabhChawla100 commented Jun 9, 2020

View reviewed changes

maropu changed the title ~~[SPARK-31941][CORE]Add the code change to handle the SparkException~~ [SPARK-31941][CORE][WEBUI] Handle SparkException in SparkUI Jun 9, 2020

SaurabhChawla100 added 2 commits June 9, 2020 16:38

Add the code change as per the review comments

b75d168

removed NoSuchElementException as SparkException is extending excepti…

99584f5

…on and it can handle this

sarutak reviewed Jun 9, 2020

View reviewed changes

throwing NoSuchElementException instead of SparkException

e8769fb

SaurabhChawla100 changed the title ~~[SPARK-31941][CORE][WEBUI] Handle SparkException in SparkUI~~ [SPARK-31941][CORE] Replace SparkException from NoSuchElementException for applicationInfo in AppStatusStore Jun 10, 2020

SaurabhChawla100 changed the title ~~[SPARK-31941][CORE] Replace SparkException from NoSuchElementException for applicationInfo in AppStatusStore~~ [SPARK-31941][CORE] Replace SparkException to NoSuchElementException for applicationInfo in AppStatusStore Jun 10, 2020

HeartSaVioR approved these changes Jun 10, 2020

View reviewed changes

sarutak approved these changes Jun 10, 2020

View reviewed changes

jiangxb1987 approved these changes Jun 10, 2020

View reviewed changes

sarutak closed this in 82ff29b Jun 10, 2020

[SPARK-31941][CORE] Replace SparkException to NoSuchElementException for applicationInfo in AppStatusStore #28768

[SPARK-31941][CORE] Replace SparkException to NoSuchElementException for applicationInfo in AppStatusStore #28768

Uh oh!

Conversation

SaurabhChawla100 commented Jun 9, 2020 • 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!

SaurabhChawla100 Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

maropu Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

maropu Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

SaurabhChawla100 Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

SaurabhChawla100 commented Jun 9, 2020

Uh oh!

maropu commented Jun 9, 2020

Uh oh!

SparkQA commented Jun 9, 2020

Uh oh!

SparkQA commented Jun 9, 2020

Uh oh!

SparkQA commented Jun 9, 2020

Uh oh!

sarutak Jun 9, 2020 • 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!

sarutak Jun 9, 2020 • 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!

SaurabhChawla100 Jun 9, 2020

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 9, 2020

Uh oh!

HeartSaVioR commented Jun 10, 2020

Uh oh!

SaurabhChawla100 commented Jun 10, 2020

Uh oh!

HeartSaVioR left a comment

Choose a reason for hiding this comment

Uh oh!

Ngone51 commented Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xccui commented Jun 10, 2020

Uh oh!

jiangxb1987 left a comment

Choose a reason for hiding this comment

Uh oh!

sarutak commented Jun 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

SaurabhChawla100 commented Jun 9, 2020 •

edited

Loading

sarutak Jun 9, 2020 •

edited

Loading

sarutak Jun 9, 2020 •

edited

Loading

Ngone51 commented Jun 10, 2020 •

edited

Loading