-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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
[SPARK-7717][WebUI] Only showing total memory and cores for alive workers #6317
Conversation
Test build #33238 has finished for PR 6317 at commit
|
Although it seems reasonable, isn't the table listing all workers regardless of state? I wonder if it's more confusing that the table won't sum to the total. Would it be better to explicitly say "Alive Workers", "Cores in use", "Memory in use" to be clear? |
@zhichao-li can you attach screenshots describing what it looks like before and after your change? |
@@ -75,6 +75,7 @@ private[ui] class MasterPage(parent: MasterWebUI) extends WebUIPage("") { | |||
|
|||
val workerHeaders = Seq("Worker Id", "Address", "State", "Cores", "Memory") | |||
val workers = state.workers.sortBy(_.id) | |||
val aliveWorkers = state.workers.filter(_.state == WorkerState.ALIVE ) |
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.
Nit: extra space at the end.
@zhichao-li I'm OK with this change but if the UI then changes to clarify that the metrics are only for alive workers. Otherwise it introduces a smaller new confusion.
OK with that @andrewor14
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.
@srowen "Alive" prefix has been added to the Worker, Cores and Memory. :)
Test build #33486 has finished for PR 6317 at commit
|
<li><strong>Alive Workers:</strong> {aliveWorkers.size}</li> | ||
<li><strong>Alive Cores:</strong> {aliveWorkers.map(_.cores).sum} Total, | ||
{aliveWorkers.map(_.coresUsed).sum} Used</li> | ||
<li><strong>Alive Memory:</strong> |
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 don't think "Alive Memory" / "Alive Cores" makes sense. "Memory in use" / "Cores in use"?
Test build #33508 has finished for PR 6317 at commit
|
What do you think @andrewor14 ? I do think it makes a bit more sense to report active workers only. |
I think this change is an improvement and low-impact in any event, so will put it in to master |
Sorry for the late response, lgtm too |
…rkers Author: zhichao.li <zhichao.li@intel.com> Closes apache#6317 from zhichao-li/workers and squashes the following commits: d68bf11 [zhichao.li] change prefix 99b6768 [zhichao.li] remove extra space and add 'Alive' prefix 1e8eb06 [zhichao.li] only showing alive workers
…rkers Author: zhichao.li <zhichao.li@intel.com> Closes apache#6317 from zhichao-li/workers and squashes the following commits: d68bf11 [zhichao.li] change prefix 99b6768 [zhichao.li] remove extra space and add 'Alive' prefix 1e8eb06 [zhichao.li] only showing alive workers
No description provided.