-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-19824][Core] Update JsonProtocol to keep consistent with the UI #18303
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
|
Test build #78054 has finished for PR 18303 at commit
|
|
Show the screenshot before and after this PR? |
|
@gatorsmile Sure, let me do it later! |
|
retest please. |
|
retest this please |
|
LGTM |
|
Test build #78154 has finished for PR 18303 at commit
|
|
retest this please |
| ("duration" -> obj.duration) | ||
| } | ||
|
|
||
| def writeApplicationDescription(obj: ApplicationDescription): JObject = { |
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.
IMO, we need to add the function descriptions for these public functions and provide a clear definition for each field. How about creating a few JIRAs and asking the open source community to do it?
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.
Let me do this at the weekend in this PR.
|
Test build #78163 has finished for PR 18303 at commit
|
| ("lastheartbeat" -> obj.lastHeartbeat) | ||
| } | ||
| /** | ||
| * Export the [[WorkerInfo]] to a Json object, a [[WorkerInfo]] consists of the information of a |
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.
, a -> . A
| * `id` a string identifier of the worker | ||
| * `host` the host that the worker is running on | ||
| * `port` the port that the worker is bound to | ||
| * `address` ${host}:${port} |
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.
If we already has host and port, why we still need the address?
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.
Good point, I'll remove this.
| * | ||
| * @return a Json object containing the following fields: | ||
| * `id` a string identifier of the application | ||
| * `starttime` time in milliseconds that the application starts |
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.
Switch the order between id and starttime?
| * @return a Json object containing the following fields: | ||
| * `id` a string identifier of the application | ||
| * `starttime` time in milliseconds that the application starts | ||
| * `name` a name describes the application |
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.
-> the description of the application
| } | ||
|
|
||
| /** | ||
| * Export the [[ApplicationInfo]] to a Json object, an [[ApplicationInfo]] consists of the |
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.
, an -> . An
| } | ||
|
|
||
| /** | ||
| * Export the [[ApplicationDescription]] to a Json object, an [[ApplicationDescription]] consists |
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.
, an -> . An
| * of the description of an application. | ||
| * | ||
| * @return a Json object containing the following fields: | ||
| * `name` a name describes the application |
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.
-> the description of the application
| * | ||
| * @return a Json object containing the following fields: | ||
| * `name` a name describes the application | ||
| * `cores` max cores can be allocated to the application, 0 means unlimited |
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.
max cores can be -> max cores that can be
| * `cores` max cores can be allocated to the application, 0 means unlimited | ||
| * `memoryperslave` minimal memory in MB required to each executor | ||
| * `user` name of the user who submitted the application | ||
| * `command` the command string that submitted the application |
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.
the command string that submitted -> the command used to submit
| } | ||
|
|
||
| /** | ||
| * Export the [[ExecutorRunner]] to a Json object, an [[ExecutorRunner]] consists of the |
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.
, an -> . An
| * information of an executor. | ||
| * | ||
| * @return a Json object containing the following fields: | ||
| * `id` a integer identifier of the executor |
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.
a integer -> an integer
| * @return a Json object containing the following fields: | ||
| * `id` a integer identifier of the executor | ||
| * `memory` memory in MB allocated to the executor | ||
| * `appid` a string identifier of the application that the executor is working for |
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.
working for -> working on
| * `memory` memory in MB allocated to the executor | ||
| * `appid` a string identifier of the application that the executor is working for | ||
| * `appdesc` a Json object of the [[ApplicationDescription]] of the application that the | ||
| * executor is working for |
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.
working for -> working on
| } | ||
|
|
||
| /** | ||
| * Export the [[DriverInfo]] to a Json object, a [[DriverInfo]] consists of the information of a |
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.
, a -> . A
| } | ||
|
|
||
| /** | ||
| * Export the [[MasterStateResponse]] to a Json object, a [[MasterStateResponse]] consists the |
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.
, a -> . A
| * `activeapps` a list of Json objects of [[ApplicationInfo]] of the active applications | ||
| * running on the master | ||
| * `completedapps` a list of Json objects of [[ApplicationInfo]] of the completed | ||
| * applications from the master |
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.
-> the applications completed in the master
|
LGTM except a few comments. |
|
Test build #78227 has finished for PR 18303 at commit
|
|
LGTM |
1 similar comment
|
LGTM |
|
Test build #78235 has finished for PR 18303 at commit
|
|
Thanks! Merging to master. |
What changes were proposed in this pull request?
Fix any inconsistent part in JsonProtocol with the UI.
This PR also contains the modifications in #17181
How was this patch tested?
Updated JsonProtocolSuite.
Before this change, localhost:8080/json shows:
After the change: