Skip to content

[SPARK-8656][WebUI] Fix the webUI and JSON API number is not synced#7038

Closed
thegiive wants to merge 4 commits into
apache:masterfrom
thegiive:SPARK-8656
Closed

[SPARK-8656][WebUI] Fix the webUI and JSON API number is not synced#7038
thegiive wants to merge 4 commits into
apache:masterfrom
thegiive:SPARK-8656

Conversation

@thegiive

Copy link
Copy Markdown

Spark standalone master web UI show "Alive Workers" total core, total used cores and "Alive workers" total memory, memory used.
But the JSON API page "http://MASTERURL:8088/json" shows "ALL workers" core, memory number.
This webUI data is not sync with the JSON API.
The proper way is to sync the number with webUI and JSON API.

@AmplabJenkins

Copy link
Copy Markdown

Can one of the admins verify this patch?

@srowen

srowen commented Jun 26, 2015

Copy link
Copy Markdown
Member

This is changing the semantics of the response though. I don't think you can necessarily do this. It is not necessarily true that what is presented in the web UI for human consumption is identical to what's in the JSON response for machine consumption.

You should ask the person who created this API to see if there is or should be a way to get just alive workers status. Is this info not in the response?

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.

Why do the filtering so many times?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I can refactor this better later. Let me find the API author first. Thanks

@thegiive

Copy link
Copy Markdown
Author

HI Sean, thanks for your opinion.

My feeling is that WebUI and API is just different presentation format. But the content should be the same. Different meaning in the same column(workers,memory,core) will confuse the user.

BTW, Let me talk to API author first :)

@thegiive

Copy link
Copy Markdown
Author

Base on git blame, I think the JSON API creator is @rxin ( 9db1e50 )

Hi Reynold

#6317 has already change some master UI's data's definition (total memory/memory in used , total core/core in used ) .
Base on the same reason list in #6317 and the WebUI should sync the number with JSON API.
I think it is reasonable to sync the JSON API's column definition.

How do you think?

@andrewor14

Copy link
Copy Markdown
Contributor

ok to test

@andrewor14

Copy link
Copy Markdown
Contributor

I think we should have the JSON protocol do whatever the normal UI does. This change looks fine.

@andrewor14

Copy link
Copy Markdown
Contributor

@thegiive have you verified that these are the only sources of mismatch?

@AmplabJenkins

Copy link
Copy Markdown

Merged build triggered.

@AmplabJenkins

Copy link
Copy Markdown

Merged build started.

@SparkQA

SparkQA commented Jun 29, 2015

Copy link
Copy Markdown

Test build #36030 has started for PR 7038 at commit 431d2b0.

@SparkQA

SparkQA commented Jun 29, 2015

Copy link
Copy Markdown

Test build #36030 has finished for PR 7038 at commit 431d2b0.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@AmplabJenkins

Copy link
Copy Markdown

Merged build finished. Test PASSed.

@andrewor14

Copy link
Copy Markdown
Contributor

Hm I just saw @srowen's comment. Actually this change is not backward compatible so I'm not sure if we can merge it. @rxin any thoughts?

@rxin

rxin commented Jul 2, 2015

Copy link
Copy Markdown
Contributor

what's the non-compatible part?

@srowen

srowen commented Jul 2, 2015

Copy link
Copy Markdown
Member

I think just that the response will have different information now -- are people relying on and properly expecting to get info for all workers in the JSON? then removing non-alive worker info is incompatible in that sense.

@rxin

rxin commented Jul 2, 2015

Copy link
Copy Markdown
Contributor

ic - to me it is almost incorrect in the past, and we are fixing a bug here ...

@AmplabJenkins

Copy link
Copy Markdown

Merged build triggered.

@AmplabJenkins

Copy link
Copy Markdown

Merged build started.

@thegiive

thegiive commented Jul 3, 2015

Copy link
Copy Markdown
Author

Hi all reviewer

I modified the code base on the comment.
Can you check if it is ok ?

@SparkQA

SparkQA commented Jul 3, 2015

Copy link
Copy Markdown

Test build #36464 has started for PR 7038 at commit 2c8ea89.

@SparkQA

SparkQA commented Jul 3, 2015

Copy link
Copy Markdown

Test build #36464 has finished for PR 7038 at commit 2c8ea89.

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

@AmplabJenkins

Copy link
Copy Markdown

Merged build finished. Test FAILed.

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.

in Spark we generally use camel case, so:

val aliveWorkers = obj.workers.filter(_.state == WorkerState.ALIVE)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi Andrew

WorkerState is org.apache.spark.deploy.master's private object. JsonProtocol cannot resolved WorkerState due to class visibility.This code cannot work unless we change WorkerState code.

I think best way to do that is add a one line method on WorkerInfo

How do you think?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

BTW, I will change the variable name to camel case without any problem

@AmplabJenkins

Copy link
Copy Markdown

Merged build triggered.

@AmplabJenkins

Copy link
Copy Markdown

Merged build started.

@SparkQA

SparkQA commented Jul 3, 2015

Copy link
Copy Markdown

Test build #36515 has started for PR 7038 at commit 9e54bf0.

@SparkQA

SparkQA commented Jul 3, 2015

Copy link
Copy Markdown

Test build #36515 has finished for PR 7038 at commit 9e54bf0.

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

@AmplabJenkins

Copy link
Copy Markdown

Merged build finished. Test PASSed.

@thegiive

thegiive commented Jul 4, 2015

Copy link
Copy Markdown
Author

HI @andrewor14

The test is passed. If there is any problem I need to change, please let me know. Thanks

@srowen

srowen commented Jul 6, 2015

Copy link
Copy Markdown
Member

LGTM

@andrewor14

Copy link
Copy Markdown
Contributor

Ok, I'm merging this into master. Thanks @thegiive.

@asfgit asfgit closed this in 9ff2033 Jul 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants