[#1401][part-1] The dashboard supports multiple Coordinator links. #1449

yl09099 · 2024-01-14T15:41:56Z

What changes were proposed in this pull request?

After multiple coordinators are deployed, a dashboard is added to link multiple coordinators.

Why are the changes needed?

Fix: #1401

Does this PR introduce any user-facing change?

No.

How was this patch tested?

No.

codecov-commenter · 2024-01-14T15:55:03Z

Codecov Report

Attention: Patch coverage is 16.00000% with 42 lines in your changes missing coverage. Please review.

Project coverage is 53.91%. Comparing base (6f6d35a) to head (5806b8f).
Report is 49 commits behind head on master.

Files	Patch %	Lines
...pache/uniffle/dashboard/web/resource/Response.java	0.00%	18 Missing ⚠️
...apache/uniffle/dashboard/web/JettyServerFront.java	0.00%	6 Missing ⚠️
...e/dashboard/web/resource/CoordinatorsResource.java	0.00%	5 Missing ⚠️
...e/uniffle/dashboard/web/proxy/WebProxyServlet.java	0.00%	4 Missing ⚠️
...e/uniffle/dashboard/web/resource/BaseResource.java	0.00%	4 Missing ⚠️
...he/uniffle/dashboard/web/utils/DashboardUtils.java	72.72%	3 Missing ⚠️
...he/uniffle/dashboard/web/resource/WebResource.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1449      +/-   ##
============================================
- Coverage     54.86%   53.91%   -0.96%     
- Complexity     2358     2947     +589     
============================================
  Files           368      440      +72     
  Lines         16379    23613    +7234     
  Branches       1504     2197     +693     
============================================
+ Hits           8986    12730    +3744     
- Misses         6862    10095    +3233     
- Partials        531      788     +257

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

zuston · 2024-01-15T03:41:41Z

Could you help review this? @xianjingfeng

xianjingfeng · 2024-01-15T08:32:18Z

dashboard/src/main/webapp/src/pages/CoordinatorServerPage.vue

+    //The system obtains data from global variables and requests the interface to obtain new data after data changes.
+    const currentServerStore= useCurrentServerStore()
+    currentServerStore.$subscribe((mutable,state)=>{
+      const headrs={"targetAddress":state.currentServer}


Is it better that passing this parameter through cookie？

Is it better that passing this parameter through cookie？

This is global. The role of cookies is not well understood.

It is not graceful to set the header before each request. Some code is redundant.

Or maybe you can use local storage and get targetAddress from the local storage before invoking the http interfaces

@yl09099 @zuston @rickyma PTAL

dashboard/src/main/java/org/apache/uniffle/dashboard/web/proxy/WebProxyServlet.java

dashboard/src/main/java/org/apache/uniffle/dashboard/web/utils/DashboardUtils.java

dashboard/src/main/java/org/apache/uniffle/dashboard/web/views/GainCoordinatorsServlet.java

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

rickyma · 2024-01-15T13:14:08Z

Can we also fix the following spelling issues in this PR?

My suggesion will be:
ResigerTime -> RegistrationTime
in files:
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

@yl09099

yl09099 · 2024-01-21T07:24:10Z

Can we also fix the following spelling issues in this PR?

My suggesion will be: ResigerTime -> RegistrationTime in files: 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

@yl09099

Have changed.

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

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

xianjingfeng · 2024-01-24T06:24:16Z

dashboard/src/main/webapp/package.json

@@ -32,6 +32,7 @@
    "core-js": "^3.8.3",
    "element-plus": "^2.3.6",
    "moment": "^2.29.4",
+    "pinia": "^2.1.7",


I just learned about pinia. Can the data in the store be shared between tabs of a browser?

I suggest remove pinia, we can just use localStorage. @yl09099 @zuston @rickyma PTAL

I suggest remove pinia, we can just use localStorage. @yl09099 @zuston @rickyma PTAL

In this case, a global store is required to monitor the address change of the entire page, and localStorage cannot be used, which cannot monitor the change of the requested address.

rickyma · 2024-04-03T14:26:22Z

No progress for this? I think we need a more powerful dashboard.

zuston · 2024-04-03T15:05:44Z

No progress for this? I think we need a more powerful dashboard.

Do you have thought on this? feel free to discuss more here

rickyma · 2024-04-04T16:48:39Z

No progress for this? I think we need a more powerful dashboard.

Do you have thought on this? feel free to discuss more here

I believe the dashboard should be updated regularly, as it provides the most intuitive experience for users. And it facilitates the management and operation of the RSS cluster for dev and ops.

I think it could be improved by:

@zuston

github-actions · 2024-04-14T11:12:33Z

Test Results

2 434 files +1 2 434 suites +1 5h 1m 41s ⏱️ + 1m 23s
935 tests +1 934 ✅ +1 1 💤 ±0 0 ❌ ±0
10 829 runs +1 10 815 ✅ +1 14 💤 ±0 0 ❌ ±0

Results for commit af382be. ± Comparison against base commit cc3f52b.

♻️ This comment has been updated with latest results.

rickyma · 2024-04-15T13:52:50Z

This is a flaky test. We can ignore this for now. I have already created an issue for this #1628.

It is ready for review? Can anyone review this? We wanna use this on production this month. @yl09099

zuston · 2024-04-16T02:31:59Z

@xianjingfeng Could you help review this?

rickyma · 2024-04-16T12:12:48Z

ping @xianjingfeng

xianjingfeng · 2024-04-16T14:08:24Z

dashboard/src/main/webapp/vue.config.js

@@ -16,4 +16,16 @@
 */

 module.exports ={
+    // 可以通过配置 vue.config.js 文件来设置代理，以将请求代理到后端服务器。


Could you translate this sentence into english?

Could you translate this sentence into english?

done.

xianjingfeng · 2024-04-16T14:17:50Z

dashboard/src/main/java/org/apache/uniffle/dashboard/web/resource/GainCoordinatorsResource.java

+
+  @GET
+  @Path("/coordinatorList")
+  public Response getCoordinatorServers() {


Suggested change

public Response getCoordinatorServers() {

public Response<Map<String, String>> getCoordinatorServers() {

dashboard/src/main/java/org/apache/uniffle/dashboard/web/resource/GainCoordinatorsResource.java

xianjingfeng · 2024-04-16T14:26:45Z

dashboard/src/main/webapp/src/router/index.js

@@ -59,6 +59,12 @@ const routes = [
        name: 'applicationpage',
        component: ApplicationPage,
    },
+    {
+        path: '/nullpage',


Is this necessary?

Is this necessary?

This is a must to prevent clicking on the drop-down box to jump to the road with links.

xianjingfeng · 2024-06-11T10:11:50Z

dashboard/src/main/java/org/apache/uniffle/dashboard/web/JettyServerFront.java

@@ -89,7 +92,9 @@ private void setRootServletHandler() {
    HandlerList handlers = new HandlerList();
    ResourceHandler resourceHandler = addResourceHandler();
    String coordinatorWebAddress = conf.getString(DashboardConf.COORDINATOR_WEB_ADDRESS);
-    ServletContextHandler servletContextHandler = addProxyHandler(coordinatorWebAddress);
+    Map<String, String> stringStringMap = DashboardUtils.convertToMap(coordinatorWebAddress);


Suggested change

Map<String, String> stringStringMap = DashboardUtils.convertToMap(coordinatorWebAddress);

Map<String, String> coordinatorAddressMap = DashboardUtils.convertToMap(coordinatorWebAddress);

xianjingfeng · 2024-06-11T10:13:58Z

dashboard/src/main/java/org/apache/uniffle/dashboard/web/resource/WebResource.java

+@Produces({MediaType.APPLICATION_JSON})
+public class WebResource {
+  @Path("coordinator")
+  public Class<CoordinatorsResource> getGainCoordinatorsResource() {


Suggested change

public Class<CoordinatorsResource> getGainCoordinatorsResource() {

public Class<CoordinatorsResource> getCoordinatorResource() {

xianjingfeng · 2024-06-11T10:14:09Z

dashboard/src/main/java/org/apache/uniffle/dashboard/web/resource/CoordinatorsResource.java

+import org.apache.hbase.thirdparty.javax.ws.rs.core.MediaType;
+
+@Produces({MediaType.APPLICATION_JSON})
+public class CoordinatorsResource extends BaseResource {


Suggested change

public class CoordinatorsResource extends BaseResource {

public class CoordinatorResource extends BaseResource {

xianjingfeng · 2024-06-11T10:17:11Z

dashboard/src/main/java/org/apache/uniffle/dashboard/web/resource/Response.java

+
+package org.apache.uniffle.dashboard.web.resource;
+
+public class Response<T> {


Could we move it to the common module? And the same class in the coordinator module.

Could we move it to the common module? And the same class in the coordinator module.

This Response is in the Coordinator module. The Coordinator module is introduced into the dashboard here, which feels a bit awkward. Just like you said, it needs to be migrated to the common module, but I don't think it should be done in this PR.

Could we move it to the common module? And the same class in the coordinator module.

This Response is in the Coordinator module. The Coordinator module is introduced into the dashboard here, which feels a bit awkward. Just like you said, it needs to be migrated to the common module, but I don't think it should be done in this PR.

I'm introducing front-end code inspection to change this.

xianjingfeng · 2024-06-11T10:17:53Z

dashboard/src/main/java/org/apache/uniffle/dashboard/web/utils/DashboardUtils.java

+  private static final Logger log = LoggerFactory.getLogger(DashboardUtils.class);
+
+  public static Map<String, String> convertToMap(String coordinatorStr) {
+    HashMap<String, String> stringMap = Maps.newHashMap();


Suggested change

HashMap<String, String> stringMap = Maps.newHashMap();

HashMap<String, String> coordinatorAddressMap = Maps.newHashMap();

xianjingfeng · 2024-06-11T10:19:24Z

dashboard/src/test/java/org/apache/uniffle/dashboard/web/utils/DashboardUtilsTest.java

+  public void testConvertToMap() {
+    String coordinatorStr =
+        "http://coordinator.hostname00:19998/,http://coordinator.hostname01:19998/,http://coordinator.hostname02:19998/,http://coordinator.hostname03:19998/";
+    Map<String, String> stringStringMap = DashboardUtils.convertToMap(coordinatorStr);


Suggested change

Map<String, String> stringStringMap = DashboardUtils.convertToMap(coordinatorStr);

Map<String, String> coordinatorAddressMap = DashboardUtils.convertToMap(coordinatorStr);

xianjingfeng · 2024-06-11T10:24:29Z

LGTM except some minor comments. Please take another look @rickyma

rickyma · 2024-06-11T11:18:41Z

dashboard/src/main/java/org/apache/uniffle/dashboard/web/resource/BaseResource.java

+  protected <T> Response<T> execute(Callable<T> callable) {
+    try {
+      return Response.success(callable.call());
+    } catch (Throwable e) {


e.getMessage(): Some exceptions stack trace will be lost. Can we log the full stack trace here?

rickyma · 2024-06-11T11:20:27Z

dashboard/src/main/java/org/apache/uniffle/dashboard/web/resource/CoordinatorResource.java

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


/coordinators is more in line with the RESTful style.

rickyma · 2024-06-11T11:22:29Z

dashboard/src/main/java/org/apache/uniffle/dashboard/web/utils/DashboardUtils.java

+import org.slf4j.LoggerFactory;
+
+public class DashboardUtils {
+  private static final Logger log = LoggerFactory.getLogger(DashboardUtils.class);


log -> LOG
Constants are usually modified with static final and named in uppercase.

rickyma · 2024-06-11T11:23:38Z

dashboard/src/main/java/org/apache/uniffle/dashboard/web/utils/DashboardUtils.java

+public class DashboardUtils {
+  private static final Logger log = LoggerFactory.getLogger(DashboardUtils.class);
+
+  public static Map<String, String> convertToMap(String coordinatorStr) {


coordinatorStr -> coordinatorAddressesStr

or

coordinatorStr -> addressesStr
if it could be other addresses, e.g. dashboard addresses/server address and so on.

rickyma · 2024-06-11T11:28:32Z

dashboard/src/main/java/org/apache/uniffle/dashboard/web/utils/DashboardUtils.java

+public class DashboardUtils {
+  private static final Logger log = LoggerFactory.getLogger(DashboardUtils.class);
+
+  public static Map<String, String> convertToMap(String coordinatorStr) {


convertToMap -> convertAddressesStrToMap

easier to be understood

rickyma · 2024-06-11T11:31:25Z

Left some minor comments. Once done, we can merge this

yl09099 · 2024-06-11T11:40:56Z

Left some minor comments. Once done, we can merge this

All done.Thanks!

rickyma · 2024-06-11T11:51:48Z

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

-export function getAllCoordinatorAddrees(params) {
-    return http.get('/coordinator/coordinatorList', params, {}, 1)
+export function getAllCoordinatorAddrees(params,headers) {
+    return http.get('/coordinator/coordinators', params, headers, 1)


This is a little bit weird.

The RESTful style should be something like:
/plural noun
/plural noun/id

e.g.
/coordinators
/coordinators/1

You can refer to https://blog.restcase.com/5-basic-rest-api-design-guidelines/.
I don't know if it is convenient for you to refactor this.

This is a little bit weird.

The RESTful style should be something like: /plural noun /plural noun/id

e.g. /coordinators /coordinators/1

You can refer to https://blog.restcase.com/5-basic-rest-api-design-guidelines/. I don't know if it is convenient for you to refactor this.

The other interfaces did not follow the restful structure, and there was a PR in the front that I wanted to use the restful structure was rejected.

/coordinator/coordinators is indeed weird. Can you try to find a solution?

rickyma · 2024-06-11T13:13:31Z

LGTM. @xianjingfeng We can merge this.

cc @zuston @jerqi

jerqi

LGTM.

yl09099 force-pushed the uniffle-1141 branch from a2f8e4e to 27ab7c7 Compare January 14, 2024 15:45

yl09099 force-pushed the uniffle-1141 branch 2 times, most recently from 0e92c00 to eecddd0 Compare January 15, 2024 02:21

zuston requested a review from xianjingfeng January 15, 2024 03:41

xianjingfeng reviewed Jan 15, 2024

View reviewed changes

dashboard/src/main/java/org/apache/uniffle/dashboard/web/proxy/WebProxyServlet.java Outdated Show resolved Hide resolved

xianjingfeng reviewed Jan 15, 2024

View reviewed changes

dashboard/src/main/java/org/apache/uniffle/dashboard/web/utils/DashboardUtils.java Outdated Show resolved Hide resolved

xianjingfeng reviewed Jan 15, 2024

View reviewed changes

dashboard/src/main/java/org/apache/uniffle/dashboard/web/views/GainCoordinatorsServlet.java Outdated Show resolved Hide resolved

xianjingfeng reviewed Jan 15, 2024

View reviewed changes

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

yl09099 force-pushed the uniffle-1141 branch from eecddd0 to 9f06a14 Compare January 21, 2024 07:22

yl09099 requested a review from xianjingfeng January 24, 2024 05:12

xianjingfeng reviewed Jan 24, 2024

View reviewed changes

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

xianjingfeng reviewed Jan 24, 2024

View reviewed changes

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

xianjingfeng reviewed Jan 24, 2024

View reviewed changes

yl09099 force-pushed the uniffle-1141 branch from 9f06a14 to b4142f6 Compare April 14, 2024 10:49

zuston requested a review from xianjingfeng April 16, 2024 02:32

xianjingfeng reviewed Apr 16, 2024

View reviewed changes

dashboard/src/main/java/org/apache/uniffle/dashboard/web/resource/GainCoordinatorsResource.java Outdated Show resolved Hide resolved

xianjingfeng reviewed Apr 16, 2024

View reviewed changes

yl09099 force-pushed the uniffle-1141 branch 2 times, most recently from 4617229 to 02dcaa1 Compare June 5, 2024 12:27

[apache#1401][part-1] The dashboard supports multiple Coordinator links.

b74f4ed

yl09099 force-pushed the uniffle-1141 branch 3 times, most recently from 5806b8f to 9c1d448 Compare June 9, 2024 14:32

[apache#1401][part-1] The dashboard supports multiple Coordinator links.

74aaaa7

yl09099 force-pushed the uniffle-1141 branch from 9c1d448 to 74aaaa7 Compare June 9, 2024 14:41

[apache#1401][part-1] The dashboard supports multiple Coordinator links.

74a798f

yl09099 force-pushed the uniffle-1141 branch from 78dc55d to 74a798f Compare June 11, 2024 05:53

[apache#1401][part-1] The dashboard supports multiple Coordinator links.

062da45

xianjingfeng reviewed Jun 11, 2024

View reviewed changes

[apache#1401][part-1] The dashboard supports multiple Coordinator links.

d6f48db

rickyma reviewed Jun 11, 2024

View reviewed changes

[apache#1401][part-1] The dashboard supports multiple Coordinator links.

366e620

rickyma reviewed Jun 11, 2024

View reviewed changes

[apache#1401][part-1] The dashboard supports multiple Coordinator links.

af382be

jerqi approved these changes Jun 12, 2024

View reviewed changes

xianjingfeng approved these changes Jun 12, 2024

View reviewed changes

xianjingfeng merged commit fe2efe0 into apache:master Jun 12, 2024
41 checks passed

	public Response getCoordinatorServers() {
	public Response<Map<String, String>> getCoordinatorServers() {

	Map<String, String> stringStringMap = DashboardUtils.convertToMap(coordinatorWebAddress);
	Map<String, String> coordinatorAddressMap = DashboardUtils.convertToMap(coordinatorWebAddress);

	public Class<CoordinatorsResource> getGainCoordinatorsResource() {
	public Class<CoordinatorsResource> getCoordinatorResource() {

	public class CoordinatorsResource extends BaseResource {
	public class CoordinatorResource extends BaseResource {


		package org.apache.uniffle.dashboard.web.resource;

		public class Response<T> {

	HashMap<String, String> stringMap = Maps.newHashMap();
	HashMap<String, String> coordinatorAddressMap = Maps.newHashMap();

[#1401][part-1] The dashboard supports multiple Coordinator links. #1449

[#1401][part-1] The dashboard supports multiple Coordinator links. #1449

Conversation

yl09099 commented Jan 14, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

codecov-commenter commented Jan 14, 2024 • edited Loading

Codecov Report

zuston commented Jan 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickyma commented Jan 15, 2024

yl09099 commented Jan 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickyma commented Apr 3, 2024

zuston commented Apr 3, 2024

rickyma commented Apr 4, 2024 • edited Loading

github-actions bot commented Apr 14, 2024 • edited Loading

Test Results

rickyma commented Apr 15, 2024

zuston commented Apr 16, 2024 • edited Loading

rickyma commented Apr 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xianjingfeng commented Jun 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickyma commented Jun 11, 2024 • edited Loading

yl09099 commented Jun 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickyma commented Jun 11, 2024

jerqi left a comment

Choose a reason for hiding this comment

yl09099 commented Jan 14, 2024 •

edited

Loading

codecov-commenter commented Jan 14, 2024 •

edited

Loading

rickyma commented Apr 4, 2024 •

edited

Loading

github-actions bot commented Apr 14, 2024 •

edited

Loading

zuston commented Apr 16, 2024 •

edited

Loading

rickyma commented Jun 11, 2024 •

edited

Loading