Skip to content

[SPARK-31632][CORE][WEBUI] Enrich the exception message when application information is unavailable #28444

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
xccui wants to merge 3 commits into from
Closed

[SPARK-31632][CORE][WEBUI] Enrich the exception message when application information is unavailable #28444

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
66b7f3f
[SPARK-31632] Add a retry logic when fetching applicationInfo
xccui May 6, 2020
0951f3d
Explicitly throws a SparkException when fail to fetch the Application…
xccui May 6, 2020
e633d1f
Remove the retry logic and add a proper exception message instead.
xccui May 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion core/src/main/scala/org/apache/spark/status/AppStatusStore.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package org.apache.spark.status

import java.util.{List => JList}
import java.util.concurrent.TimeUnit

import scala.collection.JavaConverters._
import scala.collection.mutable.HashMap
Expand All @@ -35,8 +36,21 @@ private[spark] class AppStatusStore(
val store: KVStore,
val listener: Option[AppStatusListener] = None) {

/**
* This method contains an automatic retry logic and tries to get a valid [[v1.ApplicationInfo]].
Comment thread
HyukjinKwon marked this conversation as resolved.
Outdated
* See [SPARK-31632] The ApplicationInfo in KVStore may be accessed before it's prepared
*/
def applicationInfo(): v1.ApplicationInfo = {

@jiangxb1987 jiangxb1987 May 8, 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.

Instead of make this function blocking, how about capture the Exception outside and display the proper message?

@xccui xccui May 8, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If blocking is supposed to be harmful, I'd prefer an optional result instead of throwing an exception here. In fact, due to the previous design and implementation, all the invokers of the method are expecting a valid ApplicationInfo, which means they are not capable of dealing with a None value or an exception now. If we change the returned type to Option, all the logic of the invokers (and invokers of these invokers maybe) should be somehow adjusted. There will be more design choices then.

Adding the retry logic solves the problem from the root. As I said, however, I can definitely make another choice if we reach a consensus.

@HyukjinKwon HyukjinKwon May 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 think capturing the exception outside is actually a good compromise considering the issue is rather a corner case. Let's scope narrow here since the issue is rather minor, and consider a better fix with a standard approach when any bigger issue is found in the design.

@Ngone51 Ngone51 May 11, 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.

Also sounds reasonable to me.

@xccui xccui May 12, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for your feedback, @HyukjinKwon. However, I didn't get your point (sorry for that). Did you mean changing the implementation to capturing the exception outside, or let's just keep the current approach?

@HyukjinKwon HyukjinKwon May 12, 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.

Sorry I wasn't clear. I was thinking a way, for example, as below:

-    store.view(classOf[ApplicationInfoWrapper]).max(1).iterator().next().info
+    try {
+      store.view(classOf[ApplicationInfoWrapper]).max(1).iterator().next().info
+    } catch {
+      case _: NoSuchElementException =>
+        throw new NoSuchElementException(
+          "Application information was not found. If your Spark application is " +
+            "starting up, try again when your Spark application is ready.")
+    }

@xccui xccui May 12, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks for the explanation. I'll update the PR as suggested.

store.view(classOf[ApplicationInfoWrapper]).max(1).iterator().next().info
var iterator = store.view(classOf[ApplicationInfoWrapper]).max(1).iterator()
val startTime = System.nanoTime()
while (!iterator.hasNext &&
System.nanoTime() - startTime < AppStatusStore.FETCH_APP_INFO_TIMEOUT) {
// Wait a while to see if the ApplicationInfo has been written to the store.
Thread.sleep(200)
iterator = store.view(classOf[ApplicationInfoWrapper]).max(1).iterator()
}
// The next() call may still throw a NoSuchElement exception.

@Ngone51 Ngone51 May 6, 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 think we should explicitly throw a SparkException here if the iterator is still empty to say something like "No available ApplicationInfo after waiting 5 seconds..."

iterator.next().info
}

def environmentInfo(): v1.ApplicationEnvironmentInfo = {
Expand Down Expand Up @@ -546,6 +560,11 @@ private[spark] class AppStatusStore(

private[spark] object AppStatusStore {

/**
* The timeout value used by [[AppStatusStore.applicationInfo()]].
Comment thread
HyukjinKwon marked this conversation as resolved.
Outdated
*/
private val FETCH_APP_INFO_TIMEOUT = TimeUnit.MILLISECONDS.toNanos(5000) // 5 seconds

val CURRENT_VERSION = 2L

/**
Expand Down