Skip to content

Conversation

@turboFei
Copy link
Member

@turboFei turboFei commented Nov 7, 2024

What changes were proposed in this pull request?

  1. GET /api/v1/applications/deleteApps -> DELETE /api/v1/applications

  2. GET /api/v1/applications/reviseLostShuffles -> POST /api/v1/applications/revise_lost_shuffles

Why are the changes needed?

Followup for #2746

Does this PR introduce any user-facing change?

No, these APIs has not been released yet.

How was this patch tested?

GA.

@turboFei turboFei changed the title format [CELEBORN-1601][CELEBORN-1697][FOLLOWUP] Use delete method for deleteApps and fix rest threadstack model Nov 7, 2024
@turboFei turboFei changed the title [CELEBORN-1601][CELEBORN-1697][FOLLOWUP] Use delete method for deleteApps and fix rest threadstack model [CELEBORN-1601][CELEBORN-1697][FOLLOWUP] Use delete/post methods for revise shuffle apis and fix threadstack model Nov 7, 2024
@turboFei turboFei requested review from FMX and SteNicholas November 7, 2024 19:46
@turboFei turboFei changed the title [CELEBORN-1601][CELEBORN-1697][FOLLOWUP] Use delete/post methods for revise shuffle apis and fix threadstack model [CELEBORN-1601][CELEBORN-1697][FOLLOWUP] Refine the RESTful apis for revise lost shuffles and fix threadstack model Nov 7, 2024
@turboFei turboFei changed the title [CELEBORN-1601][CELEBORN-1697][FOLLOWUP] Refine the RESTful apis for revise lost shuffles and fix threadstack model [CELEBORN-1601][CELEBORN-1697][FOLLOWUP] Refine the RESTful apis for revise lost shuffles and fix ThreadStack REST model Nov 7, 2024
@SteNicholas
Copy link
Member

@turboFei, is there any tool that could verify the consistency between generated openapi-client code and rest api model?

@turboFei turboFei force-pushed the delete_app branch 3 times, most recently from 5134b9b to 4e3bc70 Compare November 7, 2024 21:45
@turboFei
Copy link
Member Author

turboFei commented Nov 7, 2024

@turboFei, is there any tool that could verify the consistency between generated openapi-client code and rest api model?

Not yet now, maybe we can followup it to diff the generated RESTful modules in GA.

Raised https://issues.apache.org/jira/browse/CELEBORN-1699 to track

@turboFei
Copy link
Member Author

turboFei commented Nov 8, 2024

consistency between generated openapi-client code

@turboFei, is there any tool that could verify the consistency between generated openapi-client code and rest api model?

Not yet now, maybe we can followup it to diff the generated RESTful modules in GA.

Raised https://issues.apache.org/jira/browse/CELEBORN-1699 to track

Addressed in #2893

@turboFei
Copy link
Member Author

turboFei commented Nov 8, 2024

Will move the ThreadStack part into #2893

Done.

@turboFei turboFei requested a review from SteNicholas November 8, 2024 02:11
@turboFei turboFei changed the title [CELEBORN-1601][CELEBORN-1697][FOLLOWUP] Refine the RESTful apis for revise lost shuffles and fix ThreadStack REST model [CELEBORN-1601][FOLLOWUP] Refine the RESTful apis for revise lost shuffles Nov 8, 2024
SteNicholas pushed a commit that referenced this pull request Nov 8, 2024
### What changes were proposed in this pull request?
Check the consistency between openapi spec and generated openapi-client code in GA.
Fix the ThreadStack model.

### Why are the changes needed?
Address comments: #2892 (comment)

Followup for #2888
### Does this PR introduce _any_ user-facing change?
No.

### How was this patch tested?

GA.

It works:
https://github.com/apache/celeborn/actions/runs/11733693060/job/32688436233?pr=2893

<img width="1059" alt="image" src="https://github.com/user-attachments/assets/84682976-1b7d-42e0-9b62-2966f3e952d7">

After ThreadStack fixed:
<img width="1368" alt="image" src="https://github.com/user-attachments/assets/14a2a08f-dbc2-409c-a4ed-fbfee82e50b5">

Closes #2893 from turboFei/diff_openapi.

Authored-by: Wang, Fei <fwang12@ebay.com>
Signed-off-by: SteNicholas <programgeek@163.com>
@turboFei
Copy link
Member Author

turboFei commented Nov 8, 2024

rebased the code.

Copy link
Member

@SteNicholas SteNicholas left a comment

Choose a reason for hiding this comment

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

Only one minor comment.

Copy link
Contributor

@FMX FMX left a comment

Choose a reason for hiding this comment

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

Thanks. A modernized HTTP API implementation. LGTM.

Copy link
Contributor

@RexXiong RexXiong left a comment

Choose a reason for hiding this comment

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

👍 LGTM, merge to main(v0.6.0)

@RexXiong RexXiong closed this in be4c02e Nov 12, 2024
@turboFei turboFei deleted the delete_app branch November 12, 2024 02:21
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.

4 participants