YARN-11310. [Federation] Refactoring Yarn Router's Federation Web Page. #4924
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
goiri
reviewed
Sep 22, 2022
| Configuration conf = this.router.getConfig(); | ||
| boolean isEnabled = conf.getBoolean( |
There was a problem hiding this comment.
Thanks a lot for your help reviewing the code, I'll fix it.
...server-router/src/main/java/org/apache/hadoop/yarn/server/router/webapp/FederationBlock.java
Show resolved
Hide resolved
| } | ||
|
|
||
| private static void initFederationBlockTableJs(Block html, List<Map<String, String>> jsonMap) { | ||
| Gson gson =new Gson(); |
| return null; | ||
| } | ||
|
|
||
| private static void initFederationBlockTableJs(Block html, List<Map<String, String>> jsonMap) { |
There was a problem hiding this comment.
Add javadoc explaining what table this sets.
There was a problem hiding this comment.
I will add the java doc to explain the setting of this function for basic information.
| " <td>" + | ||
| " <h3>Resource Metrics</h3>" + | ||
| " <h4>Memory</h4>" + | ||
| " totalMB : '+capabilityObj.totalMB+' </p>" + |
| @@ -47,10 +48,10 @@ protected Class<? extends SubView> content() { | |||
|
|
|||
| private String rmsTableInit() { | |||
| StringBuilder b = tableInit().append(", aoColumnDefs: ["); | |||
| b.append("{'bSearchable': false, 'aTargets': [ 7 ]}") | |||
| b.append("{'bSearchable': false, 'aTargets': [ 2 ]}") | |||
There was a problem hiding this comment.
As we are at it, can we make this numbers variables with names?
| } else { | ||
| setTitle("Federation is not Enabled!"); | ||
| // When Federation is not enabled, user information needs to be prompted |
There was a problem hiding this comment.
It might be cleaner to have the if (!isEnabled) first and return after this.
There was a problem hiding this comment.
Thanks for your suggestion, I will modify the code.
|
💔 -1 overall
This message was automatically generated. |
|
@goiri Please help to review the code again, thank you very much! Modify web page style
The new page is displayed as follows: |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
goiri
reviewed
Sep 24, 2022
| } else { | ||
| setTitle("Federation is not Enabled!"); | ||
| // If Yarn Federation is not enabled. | ||
| if(!isEnabled) { |
| List<Map<String, String>> subClusterDetailMap) { | ||
| Gson gson = new Gson(); | ||
| html.script().$type("text/javascript"). | ||
| __("$(document).ready(function() { " + |
There was a problem hiding this comment.
This massive string with double embedding is very dangerous.
Isn't there a better way to have some javascript file and some html template?
There was a problem hiding this comment.
Thanks for your help in reviewing the code, I agree with you, I will add js file.
| // Binding to the FederationStateStore | ||
| FederationStateStoreFacade facade = FederationStateStoreFacade.getInstance(); | ||
|
|
||
| Map<SubClusterId, SubClusterInfo> subClustersInfo = |
| // Prepare Node | ||
| long totalNodes = subClusterInfo.getTotalNodes(); | ||
| long activeNodes = subClusterInfo.getActiveNodes(); | ||
| String nodes = String.format("<Total Nodes:%s, Active Nodes:%s>", |
| @@ -33,8 +33,9 @@ class FederationPage extends RouterView { | |||
| @Override | |||
| protected void preHead(Page.HTML<__> html) { | |||
| commonPreHead(html); | |||
| setTitle("Federation"); | |||
| setTitle("About The Federation"); | |||
There was a problem hiding this comment.
I think it makes more sense to have About YARN federation
There was a problem hiding this comment.
I will modify the code.
| @@ -34,7 +34,7 @@ public class RouterController extends Controller { | |||
|
|
|||
| @Override | |||
| public void index() { | |||
| setTitle("Router"); | |||
| setTitle("About the Router"); | |||
There was a problem hiding this comment.
I will modify the code.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
goiri
approved these changes
Sep 26, 2022
| ' AvailableVirtualCore : '+capabilityObj.availableVirtualCores+' </p>'+ | ||
| ' AllocatedVirtualCores : '+capabilityObj.allocatedVirtualCores+' </p>'+ | ||
| ' PendingVirtualCores : '+capabilityObj.pendingVirtualCores+' </p>'+ | ||
| ' <h4>Containers</h4>'+ |
There was a problem hiding this comment.
Spaces before and after the plus sign
There was a problem hiding this comment.
Thank you very much for helping to review the code, I will modify the code.
|
If the user does not configure Yarn Federation, we will give the user some help information. The page is as follows: |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@goiri Thank you very much for your help reviewing the code! |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
JIRA: YARN-11310. [Yarn Federation] Refactoring Router's Federation Web Page.
The Yarn Federation page before modification is as follows:
Enable Federation
Federation is not enabled
The page needs to be optimized as follows:
1.Title is inaccurate, should be "About The Federation"
2.If the user does not configure federation.enable, the page should prompt information instead of showing blank.
3.For cluster information, we care more about the current sub-cluster status, registration time, heartbeat time, etc., rather than capacity information. Detailed information can be displayed when clicking on the header
The Yarn Federation page after modification is as follows:
Enable federation
When we click
SC-1, we can expand the information of the cluster, and when we click SC-1 again, we can hide the information of the cluster/Federation is not enabled
We prompt the user that Federation is not turned on, and give a configuration guide link
For screenshots of the operation, please refer to the following link
https://issues.apache.org/jira/secure/attachment/13049621/Federation_Web_Page.gif