-
Notifications
You must be signed in to change notification settings - Fork 28.6k
[SPARK-12784][UI]Fix Spark UI IndexOutOfBoundsException with dynamic allocation #10728
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.
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
Conversation
thanks for picking this up so quickly. One concern I have here is that I don't want to affect application performance just because someone is viewing the UI. If the UI load blocks the application from running, that is bad. I would rather see us get a read only copy of it or something then synchronize. I'll try to look in more detail tomorrow. |
It usually doesn't affect your application performance. It only blocks the listener bus thread when rendering a page. |
Test build #49269 has finished for PR 10728 at commit
|
CC @andrewor14 |
This looks OK, and yeah we're not blocking any thread that's actually doing the scheduling or execution so it should be fine. |
@@ -52,12 +52,17 @@ private[ui] class ExecutorsPage( | |||
private val listener = parent.listener | |||
|
|||
def render(request: HttpServletRequest): Seq[Node] = { | |||
val storageStatusList = listener.storageStatusList | |||
val (storageStatusList, execInfo) = listener.synchronized { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a comment to explain why we need to do this in synchronized though, and include issue number
LGTM |
Test build #49343 has finished for PR 10728 at commit
|
I found a needless semi-colon in ExecutorsPage.scala. But it's not a big deal and we have already passed SparkQA so I think it's OK to merge without removing the semi-colon. |
Thanks, merging to master, 1.6 and 1.5 |
… allocation Add `listener.synchronized` to get `storageStatusList` and `execInfo` atomically. Author: Shixiong Zhu <shixiong@databricks.com> Closes #10728 from zsxwing/SPARK-12784. (cherry picked from commit 501e99e) Signed-off-by: Shixiong Zhu <shixiong@databricks.com>
… allocation Add `listener.synchronized` to get `storageStatusList` and `execInfo` atomically. Author: Shixiong Zhu <shixiong@databricks.com> Closes #10728 from zsxwing/SPARK-12784. (cherry picked from commit 501e99e) Signed-off-by: Shixiong Zhu <shixiong@databricks.com>
Add
listener.synchronized
to getstorageStatusList
andexecInfo
atomically.