-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page. #5030
Conversation
🎊 +1 overall
This message was automatically generated. |
@goiri Can you help review the code? Thank you very much! |
🎊 +1 overall
This message was automatically generated. |
@goiri Can you help review this pr? Thank you very much! |
appsTableData.append("\"],\n"); | ||
|
||
} catch (Exception e) { | ||
LOG.info( |
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.
One line?
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 will fix it.
"Cannot add application {}: {}", app.getAppId(), e.getMessage()); | ||
} | ||
} | ||
if (appsTableData.charAt(appsTableData.length() - 2) == ',') { |
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.
As we are refactoring, this is a little hacky.
At least it could be more readable.
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 will modify the code.
🎊 +1 overall
This message was automatically generated. |
} | ||
} | ||
|
||
// The purpose of this part of the code is to remove redundant commas. |
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.
We know how many apps there will be, we should only add it if not the last:
if (appsInfo != null) {
Collection<AppInfo> apps = appsInfo.getApps();
if (CollectionUtils.isNotEmpty(apps)) {
int numApps = apps.size();
for (AppInfo app: apps) {
...
if (i < numApps - 1) {
appsTableData.append(",");
}
}
}
}
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.
Thanks for your suggestion, I will refactor this part of the code.
🎊 +1 overall
This message was automatically generated. |
Collection<AppInfo> apps = appsInfo.getApps(); | ||
if (CollectionUtils.isNotEmpty(apps)) { | ||
int numApps = apps.size(); | ||
int i = 0; |
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++ missing?
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 put i++ after try...catch and the loop can end even if the application has errors.
} catch (Exception e) { | ||
LOG.info("Cannot add application {}: {}", app.getAppId(), e.getMessage()); | ||
} | ||
i++; |
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++ is placed after try...catch.
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.
Thanks for your suggestion, I will refactor this part of the code to make it more readable.
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 read this part of the code carefully, the original code is reasonable, we should remove the extra comma outside the loop because we can't tell where the last comma is
For example:
We have 4 apps, A, B, C, D, and we expect the result to be [A, B, C, D], but when traversing the app list, the last app D fails, and we end up with [A ,B,C,], the last app D fails, we can't handle this situation inside the loop.
But the readability of the original logic is not good, I refactored this part of the code.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@goiri Can you help to merge this pr into trunk branch? Thank you very much! I will follow up with YARN-11354 [Federation] Add Yarn Router's NodeLabel Web Page. |
@goiri Thank you very much for helping to review the code! |
JIRA: YARN-11345. [Federation] Refactoring Yarn Router's Application Web Page.
Improve the Application page of Yarn Router, try to be the same as the Yarn RM app page.
Router App page style before modification:
![image](https://user-images.githubusercontent.com/55643692/195800114-c9bb42a9-b8ee-4c04-93a5-ad09718ce4dd.png)
Router App page style after modification:
From the router's point of view, we can see the app running of the entire Federation
![image](https://user-images.githubusercontent.com/55643692/195799107-c48ee13e-862e-4796-aa28-e1fac771a77b.png)
From the subcluster point of view, we can see the app running of the subcluster
![image](https://user-images.githubusercontent.com/55643692/195799830-f09a4be9-7cc8-4f6c-905d-d120ab2b38c5.png)
For SubCluster, support query by app status
![image](https://user-images.githubusercontent.com/55643692/195800461-014ccbce-e7a6-4779-beba-ebf33000411f.png)
If we don't enable Federation
![image](https://user-images.githubusercontent.com/55643692/195800828-efc05dde-0bfe-48a4-8b47-3fe416b55e93.png)