[#960][part-1] feat(dashboard): Add some dashboard interfaces. by yl09099 · Pull Request #1053 · apache/uniffle

yl09099 · 2023-07-30T10:59:01Z

What changes were proposed in this pull request?

Add a web dashboard for uniffle, providing some dashboard apis

Why are the changes needed?

Provide data support for dashboard

Fix: #960

Does this PR introduce any user-facing change?

Users can obtain relevant metrics information through the new API

How was this patch tested?

Existing UT.

codecov-commenter · 2023-07-30T14:10:47Z

Codecov Report

Merging #1053 (c723014) into master (59e08e5) will increase coverage by 0.73%.
The diff coverage is 0.78%.

@@             Coverage Diff              @@
##             master    #1053      +/-   ##
============================================
+ Coverage     53.67%   54.40%   +0.73%     
+ Complexity     2691     2690       -1     
============================================
  Files           403      388      -15     
  Lines         23456    21218    -2238     
  Branches       1992     2003      +11     
============================================
- Hits          12589    11543    -1046     
+ Misses        10092     8972    -1120     
+ Partials        775      703      -72

Files	Coverage Δ
.../apache/uniffle/coordinator/CoordinatorServer.java	`57.46% <100.00%> (+0.31%)`	⬆️
...java/org/apache/uniffle/common/config/RssConf.java	`29.65% <0.00%> (-0.21%)`	⬇️
.../uniffle/coordinator/web/resource/APIResource.java	`0.00% <0.00%> (ø)`
...a/org/apache/uniffle/coordinator/web/Response.java	`0.00% <0.00%> (ø)`
...iffle/coordinator/web/resource/ServerResource.java	`0.00% <0.00%> (ø)`
.../uniffle/coordinator/web/vo/CoordinatorConfVO.java	`0.00% <0.00%> (ø)`
...pache/uniffle/coordinator/web/vo/UserAppNumVO.java	`0.00% <0.00%> (ø)`
...inator/web/resource/CoordinatorServerResource.java	`0.00% <0.00%> (ø)`
...g/apache/uniffle/coordinator/web/vo/AppInfoVO.java	`0.00% <0.00%> (ø)`
.../coordinator/web/resource/ApplicationResource.java	`0.00% <0.00%> (ø)`

... and 21 files with indirect coverage changes

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

advancedxy · 2023-07-31T02:20:47Z

    return this.settings.toString();
  }

+  public Set<Map.Entry<String, Object>> getAllEntry() {


Why is this needed?

advancedxy · 2023-07-31T02:27:27Z

+  public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain)
+      throws IOException, ServletException {
+    HttpServletResponse response = (HttpServletResponse) res;
+    response.setHeader("Access-Control-Allow-Origin", "*");


why cors should be enabled?

why cors should be enabled?

The port exposed on the front-end page is different from the Coordinator port. As a result, cross-domain request data may occur.

If users don't need access Coordinator API directly, we don't need to consider the problem of cross-domain. But I found in current implementation(#1055 ), users access Coordinator API directly, right? @yl09099
But as we discuss in #963 (comment), some operations will be executed in the dashboard module. @jerqi
I feel like the goal is not very clear.

I think we should discuss this thread first. WDYT? @advancedxy @jerqi @yl09099

I think we should discuss this thread first. WDYT? @advancedxy @jerqi @yl09099

What's wrong with this cross-domain approach? I'm not familiar with it.

I think we should discuss this thread first. WDYT? @advancedxy @jerqi @yl09099

If we want to collect my metrics or events, we can finish them in the dashboard module. Dashboard will help us to analyze the jobs in the future. Which job write most data? Which job give the biggest pressure? It's the most important thing for coordinator is stable enough.

But it seems that the current design can't achieve this goal. @jerqi Right? @yl09099

I think we should discuss this thread first. WDYT? @advancedxy @jerqi @yl09099

If we want to collect my metrics or events, we can finish them in the dashboard module. Dashboard will help us to analyze the jobs in the future. Which job write most data? Which job give the biggest pressure? It's the most important thing for coordinator is stable enough.

But it seems that the current design can't achieve this goal. @jerqi Right? @yl09099

Do the demonstration first, and then gradually improve the management function.

I have removed the controversial cross-domain issues, do I need to change the others? @xianjingfeng @jerqi

Could we discuss this issue at our meeting this Wednesday. @yl09099 @jerqi

Could we discuss this issue at our meeting this Wednesday. @yl09099 @jerqi

OK for me.

advancedxy · 2023-07-31T02:33:20Z

+  @Path("/{id}")
  public Response<ServerNode> node(@PathParam("id") String id) {
    return execute(() -> getClusterManager().getServerNodeById(id));
  }

  @GET
  @Path("/nodes")
-  public Response<List<ServerNode>> nodes(@QueryParam("status") String status) {
+  public Response<List<ServerNode>> nodes() {


I don't think this is the right REST api to add.

I think we have agreed to use /nodes/, '/nodes/:id` as our REST API style in the #1000

advancedxy · 2023-07-31T02:34:16Z

+    return execute(
+        () -> {
+          CoordinatorConf coordinatorConf = getCoordinatorServer().getCoordinatorConf();
+          Set<Map.Entry<String, Object>> allEntry = coordinatorConf.getAllEntry();


I see why getAllEntry is added.

But I'd rather to defined it as Map<String, String> getAll()

advancedxy · 2023-07-31T02:34:41Z

+  }
+
+  @GET
+  @Path("/nodes/{status}")


advancedxy · 2023-07-31T02:35:41Z

  }

+  @GET
+  @Path("/nodestatustotal")


Maybe /nodes/summary would be better.

advancedxy · 2023-07-31T02:50:19Z

+  private static final String LOSTNODES_URL = URL_PREFIX + "server/nodes/LOST";
+  private static final String UNHEALTHYNODES_URL = URL_PREFIX + "server/nodes/UNHEALTHY";


Like commented earlier, this would be inappropriate

advancedxy · 2023-07-31T02:50:33Z

+
+package org.apache.uniffle.coordinator.web.vo;
+
+public class AppInfoVO {


what does VO stands for? And is there any design principle for this?

VO is the data used for display, whether it is displayed in a web page, a client, or an APP, as long as this thing is seen by people, it is called VO

I think we can use org.apache.uniffle.common.Application directly.

advancedxy · 2023-07-31T02:53:02Z

+  @Context protected ServletContext servletContext;
+
+  @GET
+  @Path("/info")


Seems the api route is wrong.
This is more like coordinator conf.

The L63-L65 lines are more like a info path.

Seems the api route is wrong. This is more like coordinator conf.

The L63-L65 lines are more like a info path.

OK,Yeah, I'll fix it

advancedxy · 2023-07-31T02:55:45Z

+import org.apache.uniffle.coordinator.web.vo.UserAppNumVO;
+
+@Produces({MediaType.APPLICATION_JSON})
+public class ApplicationResource extends BaseResource {


@slfan1989 added some related applications interface in #964 , Maybe you guys should be in sync and decide which paths to added

@asfsecurity Thank you for your help reviewing the code! I discussed offline with @yl09099, we plan to merge #964 first, and then yl09099 will improve. I will improve the code of #964 in 1-2 days.

@advancedxy @jerqi Could you continue to review this for me?

xianjingfeng · 2023-08-18T06:37:59Z

+          List<AppInfoVO> userToAppList = new ArrayList<>();
+          SimpleDateFormat sdf = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
+          Map<String, Map<String, Long>> currentUserAndApp =
+              getApplicationManager().getQuotaManager().getCurrentUserAndApp();


quotaManager may be null

Do you miss this thread? @yl09099

xianjingfeng · 2023-08-18T06:41:43Z

+          Set<String> appIds = getApplicationManager().getAppIds();
+          Map<String, Integer> appTotalityMap = new HashMap<>();
+          appTotalityMap.put("appTotality", appIds.size());
+          return appTotalityMap;


How about returning appIds.size() directly?

How about returning appIds.size() directly?

It feels better to have a unique identifier on the front end to get this value.

xianjingfeng · 2023-08-18T06:47:49Z

+
+  @GET
+  @Path("/info")
+  public Response<Map> getCoodConf() {


Suggested change

public Response<Map> getCoodConf() {

public Response<Map<String, Object>> getCoordinatorConf() {

xianjingfeng · 2023-08-21T06:24:02Z

+        .filter(node -> ServerStatus.DECOMMISSIONING.equals(node.getStatus()))
+        .peek(e -> decommissioningNodes.add(e))
+        .collect(Collectors.toList());
+    return decommissioningNodes;


We can remove this line, and line 279, 282.

xianjingfeng · 2023-09-06T06:19:46Z

+    } else if (ServerStatus.DECOMMISSIONED.name().equalsIgnoreCase(status)) {
+      serverList = clusterManager.getDecommissionedServerList();
+    } else if (ServerStatus.DECOMMISSIONING.name().equalsIgnoreCase(status)) {
+      serverList = clusterManager.getDecommissioningServerList();


We can remove L74 to L77 because of #1174 .

We can remove L74 to L77 because of #1174 .

If this is removed, how can the two DECOMMISSIONED and DECOMMISSIONING servernodes be obtained?

You can take a look at the original logic in the master branch.
https://github.com/apache/incubator-uniffle/blob/f793a22dc8e5b821611b662e2a3227faac0731a2/coordinator/src/main/java/org/apache/uniffle/coordinator/web/resource/ServerResource.java#L75-L84

xianjingfeng · 2023-09-11T03:08:35Z


  public static <T> Response<T> success(T data) {
-    return new Response<>(SUCCESS_CODE, data, null);
+    return new Response<>(SUCCESS_CODE, data, "success");


Why returning success? What is the purpose?

Why returning success? What is the purpose?

This method is the success method, but the return message does not have any message, the front end can also judge the success of the message.

xianjingfeng · 2023-09-11T03:24:40Z

+
+  @GET
+  @Path("/conf")
+  public Response<List<CoordinatorConfVO>> getCoordinatorInfo() {


I think we don't need CoordinatorConfVO, Returning Response<Map<String, Object>> is better.

Suggested change

public Response<List<CoordinatorConfVO>> getCoordinatorInfo() {

public Response<List<CoordinatorConfVO>> getCoordinatorConf() {

I think we don't need CoordinatorConfVO, Returning Response<Map<String, Object>> is better.

I find the object code more readable and difficult to understand using Map form.

xianjingfeng · 2023-09-11T03:25:55Z

+
+  @GET
+  @Path("/usertotal")
+  public Response<List<UserAppNumVO>> getUserApps() {


ditto. Returning Response<Map<String, Integer>> is better.

xianjingfeng · 2023-09-11T03:26:32Z

+  }
+
+  @GET
+  @Path("/appinfos")


Suggested change

@Path("/appinfos")

@Path("/appInfos")

yl09099 · 2023-10-07T03:32:20Z

@jerqi @xianjingfeng Please trigger the compilation for me again, I didn't change anything in Tez

roryqi · 2023-10-07T03:38:22Z

@jerqi @xianjingfeng Please trigger the compilation for me again, I didn't change anything in Tez

done.

xianjingfeng

LGTM. Please take another look. @jerqi @advancedxy

@advancedxy

LGTM, @advancedxy Any another suggestion?

roryqi · 2023-11-07T11:58:08Z

Wait for two days. See @advancedxy whether has another suggestion.

roryqi · 2023-11-09T10:56:41Z

Merged. Thanks @advancedxy @yl09099 @xianjingfeng

roryqi requested a review from advancedxy July 30, 2023 13:24

yl09099 force-pushed the uniffle-960-1 branch 2 times, most recently from d02cbc2 to a8b4b43 Compare July 31, 2023 02:13

advancedxy previously requested changes Jul 31, 2023

View reviewed changes

yl09099 force-pushed the uniffle-960-1 branch 2 times, most recently from c3581b8 to 177b535 Compare August 1, 2023 02:01