[#960][part-4] feat(dashboard): Fix some display bugs and optimize the display format. #1326

yl09099 · 2023-11-23T04:43:51Z

What changes were proposed in this pull request?

1.Fixed a bug that returned data from some interface calls.
2.Modify some memory to be represented in tape units.

Why are the changes needed?

The dashboard page cannot display some indicators.

Fix: #960

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing UT.

codecov-commenter · 2023-11-23T05:08:29Z

Codecov Report

Attention: 24 lines in your changes are missing coverage. Please review.

Comparison is base (eef0134) 53.26% compared to head (88c0a84) 54.13%.
Report is 8 commits behind head on master.

Files	Patch %	Lines
...iffle/coordinator/web/resource/ServerResource.java	0.00%	16 Missing ⚠️
.../coordinator/web/resource/ApplicationResource.java	0.00%	5 Missing ⚠️
...ava/org/apache/uniffle/coordinator/ServerNode.java	0.00%	2 Missing ⚠️
...apache/uniffle/coordinator/ApplicationManager.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1326      +/-   ##
============================================
+ Coverage     53.26%   54.13%   +0.86%     
- Complexity     2712     2715       +3     
============================================
  Files           418      398      -20     
  Lines         23901    21571    -2330     
  Branches       2042     2043       +1     
============================================
- Hits          12732    11677    -1055     
+ Misses        10384     9180    -1204     
+ Partials        785      714      -71

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dashboard/src/main/webapp/src/components/ApplicationPage.vue

smallzhongfeng · 2023-11-23T11:49:26Z

coordinator/src/main/java/org/apache/uniffle/coordinator/SimpleClusterManager.java


+  @Override
+  public List<ServerNode> getDecommissioningServerList() {
+    List<ServerNode> decommissioningNodes = Lists.newArrayList();


Can we abstract a method here, because it is very similar to getDecommissionedServerList? 🤔️ @yl09099

smallzhongfeng · 2023-11-23T11:52:28Z

coordinator/src/main/java/org/apache/uniffle/coordinator/web/resource/ServerResource.java

      serverList = clusterManager.getUnhealthyServerList();
    } else if (ServerStatus.LOST.name().equalsIgnoreCase(status)) {
      serverList = clusterManager.getLostServerList();
+    } else if ("EXCLUDED".equalsIgnoreCase(status)) {


"EXCLUDED" add to ServerStatus?

xianjingfeng · 2023-11-24T01:50:03Z

coordinator/src/main/java/org/apache/uniffle/coordinator/web/resource/ServerResource.java

+          stringIntegerHash.put("lostNode", clusterManager.getLostServerList().size());
+          stringIntegerHash.put("unhealthyNode", clusterManager.getUnhealthyServerList().size());
+          stringIntegerHash.put(
+              "activeNode", clusterManager.getServerList(Collections.emptySet()).size());


getServerList is not only return active nodes.

Why do you use the earlier logic?
#1053 (comment)

Why do you use the earlier logic? #1053 (comment)

@yl09099 Are you miss this thread?

Why do you use the earlier logic? #1053 (comment)

@yl09099 Are you miss this thread?

Have been modified.

I mean why do we need to modify this part of the logic? As we discuss in #1053 (comment), If we add status in the future, we need to modify this method.

I mean why do we need to modify this part of the logic? As we discuss in #1053 (comment), If we add status in the future, we need to modify this method.

Provide a display of the number of servernode states here.

I mean why do we need to modify this part of the logic? As we discuss in #1053 (comment), If we add status in the future, we need to modify this method.

The layout of the status page also has to be changed, it doesn't have to be that way.

coordinator/src/main/java/org/apache/uniffle/coordinator/web/vo/ServerNodeVO.java

dashboard/src/main/webapp/src/components/shufflecomponent/ActiveNodeListPage.vue

dashboard/src/main/webapp/src/main.js

xianjingfeng · 2023-11-29T08:19:33Z

coordinator/src/main/java/org/apache/uniffle/coordinator/web/resource/ApplicationResource.java

+          if (getApplicationManager().getQuotaManager() != null) {
+            appTotalityMap.put(
+                "appTotality",
+                getApplicationManager().getQuotaManager().getAppIdToUser().keySet().size());


Is getApplicationManager().getAppIds().size() better?

I think all interfaces of ApplicationResource have the same problems.

The following code have the same problem. @yl09099
https://github.com/apache/incubator-uniffle/blob/800bf159479e96c6f1911f827e818d3de71e8b51/coordinator/src/main/java/org/apache/uniffle/coordinator/web/resource/ApplicationResource.java#L62
https://github.com/apache/incubator-uniffle/blob/800bf159479e96c6f1911f827e818d3de71e8b51/coordinator/src/main/java/org/apache/uniffle/coordinator/web/resource/ApplicationResource.java#L81-L83

The following code have the same problem. @yl09099

https://github.com/apache/incubator-uniffle/blob/800bf159479e96c6f1911f827e818d3de71e8b51/coordinator/src/main/java/org/apache/uniffle/coordinator/web/resource/ApplicationResource.java#L62

https://github.com/apache/incubator-uniffle/blob/800bf159479e96c6f1911f827e818d3de71e8b51/coordinator/src/main/java/org/apache/uniffle/coordinator/web/resource/ApplicationResource.java#L81-L83

Have been modified.

dashboard/src/main/webapp/package.json

dashboard/src/main/webapp/src/utils/common.js

dashboard/src/main/webapp/src/components/shufflecomponent/ActiveNodeListPage.vue

dashboard/src/main/webapp/src/components/shufflecomponent/DecommissionednodeListPage.vue

dashboard/src/main/webapp/src/components/shufflecomponent/DecommissioningNodeListPage.vue

dashboard/src/main/webapp/src/components/shufflecomponent/LostNodeList.vue

dashboard/src/main/webapp/src/components/shufflecomponent/UnhealthyNodeListPage.vue

dashboard/src/main/webapp/src/main.js

coordinator/src/main/java/org/apache/uniffle/coordinator/web/resource/ServerResource.java

dashboard/src/main/webapp/src/utils/common.js

xianjingfeng · 2023-12-01T02:30:37Z

coordinator/src/main/java/org/apache/uniffle/coordinator/web/resource/ServerResource.java

+                      clusterManager.list(),
+                      clusterManager.getLostServerList(),
+                      excludeNodes,
+                      clusterManager.getUnhealthyServerList())


clusterManager.list() contains all of unhealthy nodes and part of the excluded nodes, so, we may need use distinct()

clusterManager.list() contains all of unhealthy nodes and part of the excluded nodes, so, we may need use distinct()

Have been modified.

jerqi · 2023-12-08T01:52:15Z

@yl09099 Could you address the left comments?

xianjingfeng · 2023-12-11T02:01:57Z

coordinator/src/main/java/org/apache/uniffle/coordinator/web/resource/ApplicationResource.java

-          Set<String> appIds = getApplicationManager().getAppIds();
          Map<String, Integer> appTotalityMap = new HashMap<>();
-          appTotalityMap.put("appTotality", appIds.size());
+          if (getApplicationManager().getQuotaManager() != null) {


Suggested change

if (getApplicationManager().getQuotaManager() != null) {

xianjingfeng · 2023-12-11T02:02:13Z

coordinator/src/main/java/org/apache/uniffle/coordinator/web/resource/ApplicationResource.java

        () -> {
          List<AppInfoVO> userToAppList = new ArrayList<>();
          Map<String, Map<String, Long>> currentUserAndApp = new HashMap<>();
          if (getApplicationManager().getQuotaManager() != null) {


Suggested change

if (getApplicationManager().getQuotaManager() != null) {

Have been modified.

…ize the display format.

xianjingfeng

LGTM

xianjingfeng · 2023-12-11T06:33:31Z

Merged. Thanks @yl09099 @jerqi

…ize the display format. (apache#1326) ### What changes were proposed in this pull request? 1.Fixed a bug that returned data from some interface calls. 2.Modify some memory to be represented in tape units. ### Why are the changes needed? The dashboard page cannot display some indicators. Fix: apache#960 ### Does this PR introduce any user-facing change? No. ### How was this patch tested? Existing UT.

yl09099 force-pushed the uniffle-960-4 branch 3 times, most recently from a3f2c97 to 6c7cd3b Compare November 23, 2023 04:58

yl09099 force-pushed the uniffle-960-4 branch from 6c7cd3b to 3992b5d Compare November 23, 2023 05:23

jerqi reviewed Nov 23, 2023

View reviewed changes

dashboard/src/main/webapp/src/components/ApplicationPage.vue Outdated Show resolved Hide resolved

yl09099 force-pushed the uniffle-960-4 branch from 3992b5d to a82bf6a Compare November 23, 2023 06:02

smallzhongfeng reviewed Nov 23, 2023

View reviewed changes

xianjingfeng reviewed Nov 24, 2023

View reviewed changes

coordinator/src/main/java/org/apache/uniffle/coordinator/web/vo/ServerNodeVO.java Outdated Show resolved Hide resolved

xianjingfeng reviewed Nov 24, 2023

View reviewed changes

coordinator/src/main/java/org/apache/uniffle/coordinator/web/vo/ServerNodeVO.java Outdated Show resolved Hide resolved

xianjingfeng reviewed Nov 24, 2023

View reviewed changes

dashboard/src/main/webapp/src/components/shufflecomponent/ActiveNodeListPage.vue Outdated Show resolved Hide resolved

yl09099 force-pushed the uniffle-960-4 branch from a82bf6a to 20d9dd4 Compare November 24, 2023 06:40

yl09099 requested review from jerqi, smallzhongfeng and xianjingfeng November 27, 2023 02:18

yl09099 force-pushed the uniffle-960-4 branch 4 times, most recently from 34d85ed to 2758abc Compare November 29, 2023 07:53

xianjingfeng reviewed Nov 29, 2023

View reviewed changes

dashboard/src/main/webapp/src/main.js Outdated Show resolved Hide resolved

xianjingfeng reviewed Nov 29, 2023

View reviewed changes

dashboard/src/main/webapp/package.json Outdated Show resolved Hide resolved

yl09099 force-pushed the uniffle-960-4 branch 3 times, most recently from e7bc258 to e3ab43f Compare November 29, 2023 08:45

xianjingfeng reviewed Nov 29, 2023

View reviewed changes

dashboard/src/main/webapp/src/utils/common.js Outdated Show resolved Hide resolved

yl09099 force-pushed the uniffle-960-4 branch 2 times, most recently from e5c614f to 800bf15 Compare November 30, 2023 03:08

yl09099 requested a review from xianjingfeng November 30, 2023 03:50