Skip to content

[#999] improvement: apply REST principles in the urls of the http interfaces - #1000

Merged
roryqi merged 9 commits into
apache:masterfrom
xianjingfeng:issue_999
Jul 17, 2023
Merged

[#999] improvement: apply REST principles in the urls of the http interfaces#1000
roryqi merged 9 commits into
apache:masterfrom
xianjingfeng:issue_999

Conversation

@xianjingfeng

@xianjingfeng xianjingfeng commented Jul 11, 2023

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Apply REST principles in the urls of the http interfaces

Why are the changes needed?

Fix: #999

Does this PR introduce any user-facing change?

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

Decommission single shuffle server example:

curl -XPOST -H "Content-type:application/json" "http://127.0.0.1:19998/api/server/127.0.0.1-19999/decommission" 

Cancel decommission single shuffle server example:

curl -XPOST -H "Content-type:application/json" "http://127.0.0.1:19998/api/server/127.0.0.1-19999/cancelDecommission" 

Fetch single shuffle server:

curl -X GET http://127.0.0.1:19998/api/server/127.0.0.1-19999

Fetch server list:

#path: /api/server/nodes/[{status}]
curl  "http://127.0.0.1:19998/api/server/nodes"
curl  "http://127.0.0.1:19998/api/server/nodes/DECOMMISSIONING"
curl  "http://127.0.0.1:19998/api/server/nodes/ACTIVE"

How was this patch tested?

UT

@codecov-commenter

codecov-commenter commented Jul 11, 2023

Copy link
Copy Markdown

Codecov Report

Merging #1000 (71f7309) into master (411b445) will increase coverage by 1.17%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master    #1000      +/-   ##
============================================
+ Coverage     53.67%   54.84%   +1.17%     
- Complexity     2523     2525       +2     
============================================
  Files           381      362      -19     
  Lines         21648    19291    -2357     
  Branches       1800     1799       -1     
============================================
- Hits          11620    10581    -1039     
+ Misses         9322     8078    -1244     
+ Partials        706      632      -74     
Impacted Files Coverage Δ
...niffle/coordinator/web/resource/AdminResource.java 0.00% <0.00%> (ø)
...uniffle/coordinator/web/resource/BaseResource.java 0.00% <0.00%> (ø)
...iffle/coordinator/web/resource/ServerResource.java 0.00% <0.00%> (ø)

... and 24 files with indirect coverage changes

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

@roryqi

roryqi commented Jul 11, 2023

Copy link
Copy Markdown
Contributor

@slfan1989 @yl09099 Will this change affect the module cli? Do we need another modification for the module cli?

@yl09099

yl09099 commented Jul 11, 2023

Copy link
Copy Markdown
Contributor

@slfan1989 @yl09099 Will this change affect the module cli? Do we need another modification for the module cli?

Mine doesn't. That's what I'm trying to do

@roryqi

roryqi commented Jul 11, 2023

Copy link
Copy Markdown
Contributor

@Kwafoor

xianjingfeng added 5 commits July 14, 2023 22:45
This reverts commit c993c62.
# Conflicts:
#	common/src/test/java/org/apache/uniffle/common/metrics/TestUtils.java
#	coordinator/src/main/java/org/apache/uniffle/coordinator/web/resource/AdminResource.java
#	coordinator/src/main/java/org/apache/uniffle/coordinator/web/resource/ServerResource.java
#	integration-test/common/src/test/java/org/apache/uniffle/test/ServletTest.java
Comment thread docs/coordinator_guide.md Outdated
### Fetch shuffle servers

<details>
<summary><code>GET</code> <code><b>/api/server/nodes/{status}</b></code> </summary>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you update this part accordingly?

status should be a query parameter, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@advancedxy advancedxy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, except some comments regarding documentation.

@advancedxy advancedxy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

The ut has one failure, maybe there's some flaky test.

@xianjingfeng

Copy link
Copy Markdown
Member Author

The ut has one failure, maybe there's some flaky test.

Yes. #409

@roryqi
roryqi merged commit e17b24f into apache:master Jul 17, 2023
@xianjingfeng
xianjingfeng deleted the issue_999 branch August 9, 2024 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

5 participants