[#80][Part-3] feat: add REST API for decommisson #684

xianjingfeng · 2023-03-04T08:36:48Z

What changes were proposed in this pull request?

Add REST API for decommisson

Why are the changes needed?

Support shuffle server decommission. It is a part of #80

Does this PR introduce any user-facing change?

Env:

Server IP: 127.0.0.1
HTTP port: 19998
RPC port: 19999

Decommission example:

curl -XPOST -H "Content-type:application/json" "http://127.0.0.1:19998/api/server/decommission" -d '{"serverIds:": ["127.0.0.1:19999"]}'

Cancel decommission example:

curl -XPOST -H "Content-type:application/json" "http://127.0.0.1:19998/api/server/cancelDecommission" -d '{"serverIds:": ["127.0.0.1:19999"]}'

Get server list:

# path: /api/server/nodes[?id={serverId}][?status={serverStatus}]
curl  "http://127.0.0.1:19998/api/server/nodes?status=DECOMMISSIONING"
curl  "http://127.0.0.1:19998/api/server/nodes?status=ACTIVE"

How was this patch tested?

UT

# Conflicts: # coordinator/src/main/java/org/apache/uniffle/coordinator/SimpleClusterManager.java

# Conflicts: # common/src/main/java/org/apache/uniffle/common/ServerStatus.java # common/src/main/java/org/apache/uniffle/common/exception/InvalidRequestException.java # common/src/main/java/org/apache/uniffle/common/rpc/StatusCode.java # common/src/test/java/org/apache/uniffle/common/ServerStatusTest.java # coordinator/src/main/java/org/apache/uniffle/coordinator/CoordinatorGrpcService.java # coordinator/src/main/java/org/apache/uniffle/coordinator/CoordinatorServer.java # coordinator/src/main/java/org/apache/uniffle/coordinator/ServerNode.java # coordinator/src/main/java/org/apache/uniffle/coordinator/SimpleClusterManager.java # coordinator/src/test/java/org/apache/uniffle/coordinator/ServerNodeTest.java # integration-test/common/src/test/java/org/apache/uniffle/test/ShuffleServerInternalGrpcTest.java # internal-client/src/main/java/org/apache/uniffle/client/api/ShuffleServerInternalClient.java # internal-client/src/main/java/org/apache/uniffle/client/impl/grpc/ShuffleServerInternalGrpcClient.java # internal-client/src/main/java/org/apache/uniffle/client/request/RssDecommissionRequest.java # internal-client/src/main/java/org/apache/uniffle/client/response/RssDecommissionResponse.java # proto/src/main/proto/Rss.proto # server/src/main/java/org/apache/uniffle/server/ShuffleServer.java # server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java # server/src/main/java/org/apache/uniffle/server/ShuffleServerInternalGrpcService.java

codecov-commenter · 2023-03-04T08:44:36Z

Codecov Report

Merging #684 (fddc053) into master (313ddd6) will increase coverage by 2.19%.
The diff coverage is 18.88%.

@@             Coverage Diff              @@
##             master     #684      +/-   ##
============================================
+ Coverage     60.79%   62.99%   +2.19%     
- Complexity     1839     1858      +19     
============================================
  Files           221      214       -7     
  Lines         12655    10774    -1881     
  Branches       1069     1065       -4     
============================================
- Hits           7694     6787     -907     
+ Misses         4553     3639     -914     
+ Partials        408      348      -60

Impacted Files	Coverage Δ
...a/org/apache/uniffle/coordinator/web/Response.java	`0.00% <0.00%> (ø)`
...dinator/web/request/CancelDecommissionRequest.java	`0.00% <0.00%> (ø)`
...e/coordinator/web/request/DecommissionRequest.java	`0.00% <0.00%> (ø)`
...e/uniffle/coordinator/web/servlet/BaseServlet.java	`10.00% <10.00%> (ø)`
.../uniffle/coordinator/web/servlet/NodesServlet.java	`20.00% <20.00%> (ø)`
...dinator/web/servlet/CancelDecommissionServlet.java	`27.27% <27.27%> (ø)`
...e/coordinator/web/servlet/DecommissionServlet.java	`27.27% <27.27%> (ø)`
.../apache/uniffle/coordinator/CoordinatorServer.java	`59.37% <100.00%> (+1.99%)`	⬆️
...ategy/storage/AppBalanceSelectStorageStrategy.java	`70.00% <0.00%> (-2.00%)`	⬇️
...org/apache/uniffle/server/LocalStorageChecker.java	`68.14% <0.00%> (-0.73%)`	⬇️
... and 19 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

This reverts commit d4a3fc6.

This reverts commit bc5f988.

zuston · 2023-03-04T11:37:34Z

From the example listed in description, I have questions

Do we support decommission multiple server in one request?
Can we list all decommissioning and decommissioned servers?

xianjingfeng · 2023-03-04T12:50:53Z

From the example listed in description, I have questions

Do we support decommission multiple server in one request?

Can we list all decommissioning and decommissioned servers?

Updated. Now supported.

advancedxy

The code LGTM generally except one meta question.

common/src/test/java/org/apache/uniffle/common/metrics/TestUtils.java

advancedxy · 2023-03-05T13:16:44Z

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

@@ -179,6 +183,19 @@ private void initialization() throws Exception {
    server = coordinatorFactory.getServer();
  }

+  private void registerRESTAPI() throws Exception {
+    LOG.info("Register REST API");
+    jettyServer.addServlet(


How do you think to struct REST api as follows:

GET /api/v1/servers # list all servers. GET /api/v1/servers/:id # get the specific server with id. POST /api/v1/servers/:id/decommission # start decommission of server with id POST /api/v1/servers/:id/cancelDecommission #... POST /api/v1/servers/decommission # batch decommission. similar to the current impl POST /api/v1/servers/cancelDecommission # batch cancel decommission.

It is troublesome to get parameters from path in the current framework. Should we introduce other REST frameworks?

Should we introduce other REST frameworks?

If it's too much trouble, I'm OK with current route path. If it's simple, I prefer the above REST path, which is more idiomatic. It's up to you.

There will be more and more rest apis in the future. It is still troublesome to add an api under the current framework. So I think we can consider it later. How about introduce Jersey? @advancedxy @jerqi @zuston @kaijchen

I'm not familiar with Jersey. Is it lightweight enough? Is it used widely?

I'm not familiar with Jersey. Is it lightweight enough? Is it used widely?

Yes. Hadoop and Spark use Jersey either.

I don't know if Jersey is lightweight. I hope the restful framework could be modern and lightweight.

There will be more and more rest apis in the future. It is still troublesome to add an api under the current framework. So I think we can consider it later. How about introduce Jersey? @advancedxy @jerqi @zuston @kaijchen

I'm ok with current status. Let's make the new REST server framework a followup PR. Jersey itself seems a bit complex, but the path annotation https://eclipse-ee4j.github.io/jersey.github.io/documentation/latest/user-guide.html#d0e2081 is quite elegant.

Maybe we can refer to Apache Livy, it use scalatra. We can use similar Java framework.
https://www.51cto.com/article/311366.html

jerqi · 2023-03-08T06:27:57Z

@smallzhongfeng Could you help me review this pr?

xianjingfeng · 2023-03-08T13:39:18Z

Env: Server IP: 127.0.0.1 HTTP port: 19998 RPC port: 19999
Decommission example: curl -XPOST -H "Content-type:application/json" "http://127.0.0.1:19998/api/server/decommission" -d '{"serverIds:": ["127.0.0.1:19999"]}'
Cancel decommission example: curl -XPOST -H "Content-type:application/json" "http://127.0.0.1:19998/api/server/cancelDecommission" -d '{"serverIds:": ["127.0.0.1:19999"]}'
Get server list: /api/server/nodes[?id={serverId}][?status={serverStatus}] curl "http://127.0.0.1:19998/api/server/nodes?status=DECOMMISSIONING" curl "http://127.0.0.1:19998/api/server/nodes?status=ACTIVE"

Thanks @xianjingfeng for the work. I suggest adding some docs for this, in this PR or part-4.

Done

docs/coordinator_guide.md

coordinator/src/main/java/org/apache/uniffle/coordinator/web/request/DecommissionRequest.java

coordinator/src/main/java/org/apache/uniffle/coordinator/web/servlet/BaseServlet.java

smallzhongfeng · 2023-03-09T06:17:52Z

The GA failure cause by CoordinatorMetricsTest, could you rebase the latest code and trigger test again ?

smallzhongfeng

LGTM. 👍

kaijchen · 2023-03-10T08:00:09Z

Thanks @xianjingfeng for the work, and thanks everyone reviewing.

### What changes were proposed in this pull request? Add REST API for decommisson ### Why are the changes needed? Support shuffle server decommission. It is a part of apache#80 ### Does this PR introduce _any_ user-facing change? Env: * Server IP: 127.0.0.1 * HTTP port: 19998 * RPC port: 19999 Decommission example: ```shell curl -XPOST -H "Content-type:application/json" "http://127.0.0.1:19998/api/server/decommission" -d '{"serverIds:": ["127.0.0.1:19999"]}' ``` Cancel decommission example: ```shell curl -XPOST -H "Content-type:application/json" "http://127.0.0.1:19998/api/server/cancelDecommission" -d '{"serverIds:": ["127.0.0.1:19999"]}' ``` Get server list: ```shell # path: /api/server/nodes[?id={serverId}][?status={serverStatus}] curl "http://127.0.0.1:19998/api/server/nodes?status=DECOMMISSIONING" curl "http://127.0.0.1:19998/api/server/nodes?status=ACTIVE" ``` ### How was this patch tested? UT

xianjingfeng added 14 commits February 2, 2023 18:06

Temporary submission

21f1e17

Merge branch 'master' into issue_80_2

cdf91fc

Temporary submission

6ef0ba6

Temporary submission

5663623

Merge branch 'master' into issue_80_2

6b62ffc

Complete the rpc section

8559119

Optimize

6d433ae

Merge branch 'master' into issue_80_2

376320f

# Conflicts: # coordinator/src/main/java/org/apache/uniffle/coordinator/SimpleClusterManager.java

Fix code style.

5e33f33

Optimize

4d1f539

Optimize

ffa3456

Optimize

7b37fa7

Fix UT

ea37b61

xianjingfeng requested review from jerqi, advancedxy, kaijchen and zuston March 4, 2023 08:37

xianjingfeng changed the title ~~[#80][Part-2] feat: add REST API for decommisson~~ [#80][Part-3] feat: add REST API for decommisson Mar 4, 2023

xianjingfeng added 5 commits March 4, 2023 18:59

test

bc5f988

test

d4a3fc6

Revert "test"

c4a5e2a

This reverts commit d4a3fc6.

Revert "test"

5f6b66b

This reverts commit bc5f988.

test

840b740

testOptimize

c8e5ae0

Fix license

d61b6ce

advancedxy reviewed Mar 5, 2023

View reviewed changes

jerqi requested a review from smallzhongfeng March 8, 2023 06:27

Add docs

c64aaa7

Add docsOptimize

f0e1ef5

advancedxy reviewed Mar 8, 2023

View reviewed changes

docs/coordinator_guide.md Show resolved Hide resolved

docs/coordinator_guide.md Outdated Show resolved Hide resolved

advancedxy reviewed Mar 8, 2023

View reviewed changes

docs/coordinator_guide.md Outdated Show resolved Hide resolved

smallzhongfeng reviewed Mar 8, 2023

View reviewed changes

Optimize

fddc053

xianjingfeng requested review from smallzhongfeng and advancedxy March 9, 2023 02:24

advancedxy reviewed Mar 9, 2023

View reviewed changes

coordinator/src/main/java/org/apache/uniffle/coordinator/web/servlet/BaseServlet.java Show resolved Hide resolved

smallzhongfeng approved these changes Mar 9, 2023

View reviewed changes

advancedxy approved these changes Mar 9, 2023

View reviewed changes

jerqi approved these changes Mar 9, 2023

View reviewed changes

kaijchen approved these changes Mar 10, 2023

View reviewed changes

kaijchen merged commit 4b5bf10 into apache:master Mar 10, 2023

xianjingfeng deleted the issue_80_part3 branch March 10, 2023 09:13

jerqi mentioned this pull request May 10, 2023

[Improvement] Introduce Jersey to strengthen REST API #864

Closed

3 tasks

jerqi referenced this pull request in beryllw/incubator-uniffle May 27, 2023

[apache#768] feat(cli): Cli method for blacklist update

3c6fc01

xianjingfeng mentioned this pull request Jun 18, 2023

[#768] feat(cli): Cli method for blacklist update #931

Merged

xianjingfeng mentioned this pull request Jul 11, 2023

[Improvement] apply REST principles in the urls of the http interfaces #999

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#80][Part-3] feat: add REST API for decommisson #684

[#80][Part-3] feat: add REST API for decommisson #684

xianjingfeng commented Mar 4, 2023 •

edited by kaijchen

codecov-commenter commented Mar 4, 2023 •

edited

zuston commented Mar 4, 2023

xianjingfeng commented Mar 4, 2023

advancedxy left a comment

advancedxy Mar 5, 2023

xianjingfeng Mar 6, 2023

advancedxy Mar 6, 2023

xianjingfeng Mar 7, 2023

jerqi Mar 7, 2023

xianjingfeng Mar 8, 2023

jerqi Mar 8, 2023

zuston Mar 8, 2023

advancedxy Mar 8, 2023

jerqi Mar 8, 2023

jerqi commented Mar 8, 2023

xianjingfeng commented Mar 8, 2023

smallzhongfeng commented Mar 9, 2023

smallzhongfeng left a comment

kaijchen commented Mar 10, 2023

[#80][Part-3] feat: add REST API for decommisson #684

[#80][Part-3] feat: add REST API for decommisson #684

Conversation

xianjingfeng commented Mar 4, 2023 • edited by kaijchen

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 Mar 4, 2023 • edited

Codecov Report

zuston commented Mar 4, 2023

xianjingfeng commented Mar 4, 2023

advancedxy left a 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

jerqi commented Mar 8, 2023

xianjingfeng commented Mar 8, 2023

smallzhongfeng commented Mar 9, 2023

smallzhongfeng left a comment

Choose a reason for hiding this comment

kaijchen commented Mar 10, 2023

xianjingfeng commented Mar 4, 2023 •

edited by kaijchen

codecov-commenter commented Mar 4, 2023 •

edited