[CELEBORN-1317] Refine celeborn http server and support swagger ui by turboFei · Pull Request #2371 · apache/celeborn

turboFei · 2024-03-09T02:55:01Z

What changes were proposed in this pull request?

Before, there is no http request spec likes query param, http method and response mediaType.
And for each api, a HttpEndpoint class is needed.

In this PR, we refine the code for http service and provide swagger ui.

Note that: This pr does not change the orignal api request and response behavior, including metrics APIs.

TODO:

define DTO
http request authentication

Why are the changes needed?

To close CELEBORN-1317

Does this PR introduce any user-facing change?

The api is align with before.

How was this patch tested?

UT.

codecov · 2024-03-09T07:18:28Z

Codecov Report

Attention: Patch coverage is 8.97959% with 223 lines in your changes are missing coverage. Please review.

Project coverage is 38.55%. Comparing base (b3eed34) to head (8d715ff).
Report is 22 commits behind head on main.

Files	Patch %	Lines
...pache/celeborn/server/common/http/HttpServer.scala	0.00%	52 Missing ⚠️
...apache/celeborn/server/common/http/HttpUtils.scala	0.00%	42 Missing ⚠️
...rver/common/http/api/CelebornOpenApiResource.scala	0.00%	33 Missing ⚠️
...rg/apache/celeborn/server/common/HttpService.scala	0.00%	26 Missing ⚠️
...eborn/server/common/http/api/ApiBaseResource.scala	0.00%	13 Missing ⚠️
...eleborn/server/common/http/api/OpenAPIConfig.scala	0.00%	10 Missing ⚠️
...eborn/server/common/http/api/ApiRootResource.scala	0.00%	9 Missing ⚠️
...cala/org/apache/celeborn/common/CelebornConf.scala	73.34%	4 Missing and 4 partials ⚠️
...orn/server/common/http/api/ApiRequestContext.scala	0.00%	8 Missing ⚠️
.../apache/celeborn/common/metrics/MetricsUtils.scala	0.00%	7 Missing ⚠️
... and 7 more

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2371       +/-   ##
===========================================
- Coverage   48.72%   38.55%   -10.17%     
===========================================
  Files         209      213        +4     
  Lines       13014    13105       +91     
  Branches     1121     1134       +13     
===========================================
- Hits         6340     5051     -1289     
- Misses       6263     7762     +1499     
+ Partials      411      292      -119

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

turboFei · 2024-03-09T22:03:09Z

cc @RexXiong

service/src/main/scala/org/apache/celeborn/server/common/HttpService.scala

turboFei · 2024-03-12T04:57:32Z

have made a self check.

Add metrics handlers
remove javax.servlet-api and prefer jakarta.servlet-api
move metrics prometheus/json handlers from common to service module to prevent impacting the client dependencies

cc @RexXiong @waitinfuture @cfmcgrady

cfmcgrady · 2024-03-12T13:57:41Z

Perhaps we could consider loading the swagger-ui static resources from swagger-ui.jar, similar to how Kyuubi does it. As there would be no requirement to modify the HTML files, this approach would simplify the maintenance of the swagger-ui resources. cc @pan3793

turboFei · 2024-03-12T18:36:58Z

thanks @cfmcgrady addressed

turboFei · 2024-03-12T18:50:27Z

Confirmed, works well

waitinfuture

Went over this, generally LGTM, thanks! cc @AngersZhuuuu

waitinfuture · 2024-03-13T08:15:33Z

dev/deps/dependencies-server

 jackson-annotations/2.15.3//jackson-annotations-2.15.3.jar
 jackson-core/2.15.3//jackson-core-2.15.3.jar
 jackson-databind/2.15.3//jackson-databind-2.15.3.jar
+jackson-dataformat-yaml/2.13.2//jackson-dataformat-yaml-2.13.2.jar


Lot of new dependencies added, are all of these required?

Seems there is some bug for maven dependencies

For a dependency with test scope, it will copy the sub dependencies of that test dependency.

turboFei · 2024-03-18T23:46:52Z

pom.xml

+          <exclusion>
+            <groupId>jakarta.activation</groupId>
+            <artifactId>jakarta.activation-api</artifactId>
+          </exclusion>


FYI:
@waitinfuture @pan3793
8d715ff
I exclude jakarta.activation-api from the test scope dependency jersey-test-framework-core.

Then the jakarta.activation-api disappear from dependencies-server.

It does not make senses, right?

but the finally dependencyList of maven and sbt are the same.

I checked the existing dependencies involved in this pr, all of them appear in the kyuubi dependency list.

So, I think the dependencies looks fine.

add swagger base resource align master api save saev format remove unused licenese format deps exclude save dep fix dep build save fix refine dep worker deps fix remove docs stop http service remove unused master retry remove jetty from mr description refine Revert "master retry" This reverts commit 94a760f. Revert "remove jetty from mr" This reverts commit bfcbf0b. fix dep more ut remove unused shutdown hook revert unused docs typo Revert "revert unused" This reverts commit 4fe53c6. refine deps fix dep mr fix worker metrics fix scala 2.13 move to service module fix dep fix deps add license Jetty UI (#3) Using static resource from swagger-ui jar remove files dep exclude jakarta.activation-api exclude

RexXiong

LGTM, thanks, PTAL @SteNicholas @cfmcgrady

turboFei · 2024-03-27T06:49:44Z

https://github.com/apache/celeborn/actions/runs/8447675312/job/23138413282?pr=2371

- celeborn flink integration test - word count *** FAILED ***
  /home/runner/work/celeborn/celeborn/tests/flink-it/target/tmp/celeborn-872038908961646373/celeborn-worker/shuffle_data/1711521828518-1fce5a76b6dfbf104c5d7a224cdbc165/3/2-0-0 had length 0 instead of expected length 177 (WordCountTest.scala:90)
24/03/27 06:43:53,720 ERROR [data-client-137-1] TransportResponseHandler: Still have 144 requests outstanding when connection from localhost/127.0.0.1:39701 is closed
24/03/27 06:43:53,754 ERROR [data-client-137-3] TransportResponseHandler: Still have 128 requests outstanding when connection from localhost/127.0.0.1:33761 is closed
24/03/27 06:43:53,791 ERROR [data-client-137-2] TransportResponseHandler: Still have 120 requests outstanding when connection from localhost/127.0.0.1:43501 is closed
Run completed in 2 minutes, 22 seconds.
Total number of tests run: 5
Suites: completed 4, aborted 0
Tests: succeeded 4, failed 1, canceled 0, ignored 0, pending 0
*** 1 TEST FAILED ***

RexXiong · 2024-03-27T07:14:21Z

https://github.com/apache/celeborn/actions/runs/8447675312/job/23138413282?pr=2371

- celeborn flink integration test - word count *** FAILED ***
  /home/runner/work/celeborn/celeborn/tests/flink-it/target/tmp/celeborn-872038908961646373/celeborn-worker/shuffle_data/1711521828518-1fce5a76b6dfbf104c5d7a224cdbc165/3/2-0-0 had length 0 instead of expected length 177 (WordCountTest.scala:90)
24/03/27 06:43:53,720 ERROR [data-client-137-1] TransportResponseHandler: Still have 144 requests outstanding when connection from localhost/127.0.0.1:39701 is closed
24/03/27 06:43:53,754 ERROR [data-client-137-3] TransportResponseHandler: Still have 128 requests outstanding when connection from localhost/127.0.0.1:33761 is closed
24/03/27 06:43:53,791 ERROR [data-client-137-2] TransportResponseHandler: Still have 120 requests outstanding when connection from localhost/127.0.0.1:43501 is closed
Run completed in 2 minutes, 22 seconds.
Total number of tests run: 5
Suites: completed 4, aborted 0
Tests: succeeded 4, failed 1, canceled 0, ignored 0, pending 0
*** 1 TEST FAILED ***

I will rerun those jobs.

RexXiong · 2024-03-27T15:15:50Z

Thanks, merge to main(v0.5.0)

turboFei · 2024-03-27T15:23:49Z

thanks all

…ange since 0.5.0 ### What changes were proposed in this pull request? Add migration doc for RESTful api change for celeborn 0.5.0. ### Why are the changes needed? There was a typo in #2371, the `/shuffles` api was renamed to `/shuffle`. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? GA. Closes #2960 from turboFei/shuffles_api. Authored-by: Wang, Fei <fwang12@ebay.com> Signed-off-by: SteNicholas <programgeek@163.com>

turboFei force-pushed the jetty branch 5 times, most recently from dee7aa1 to ba14b9d Compare March 9, 2024 07:18

turboFei force-pushed the jetty branch 2 times, most recently from bd6b76c to 6a06770 Compare March 9, 2024 07:52

turboFei changed the title ~~Jetty~~ [CELEBORN-1317] Refine celeborn http server and show swagger info Mar 9, 2024

turboFei changed the title ~~[CELEBORN-1317] Refine celeborn http server and show swagger info~~ [CELEBORN-1317] Refine celeborn http server and support to show swagger info Mar 9, 2024

turboFei force-pushed the jetty branch 2 times, most recently from 557a55c to c0313ec Compare March 9, 2024 19:15

turboFei changed the title ~~[CELEBORN-1317] Refine celeborn http server and support to show swagger info~~ [CELEBORN-1317] Refine celeborn http server and support swagger ui Mar 10, 2024

turboFei force-pushed the jetty branch from 2b4389b to 4fe53c6 Compare March 11, 2024 16:51

turboFei commented Mar 11, 2024

View reviewed changes

service/src/main/scala/org/apache/celeborn/server/common/HttpService.scala Show resolved Hide resolved

waitinfuture reviewed Mar 13, 2024

View reviewed changes

turboFei force-pushed the jetty branch from e315d87 to 8d715ff Compare March 18, 2024 23:44

turboFei commented Mar 18, 2024

View reviewed changes

turboFei force-pushed the jetty branch from 8d715ff to a53fcb5 Compare March 27, 2024 06:36

RexXiong approved these changes Mar 27, 2024

View reviewed changes

cfmcgrady approved these changes Mar 27, 2024

View reviewed changes

RexXiong closed this in adbc77c Mar 27, 2024

turboFei deleted the jetty branch November 28, 2024 03:15

turboFei mentioned this pull request Nov 28, 2024

[CELEBORN-1752] Migration guide for unexpected shuffle RESTful api change since 0.5.0 #2960

Closed

Conversation

turboFei commented Mar 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

codecov bot commented Mar 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

turboFei commented Mar 9, 2024

Uh oh!

Uh oh!

turboFei commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cfmcgrady commented Mar 12, 2024

Uh oh!

turboFei commented Mar 12, 2024

Uh oh!

turboFei commented Mar 12, 2024

Uh oh!

waitinfuture left a comment

Choose a reason for hiding this comment

Uh oh!

waitinfuture Mar 13, 2024

Choose a reason for hiding this comment

Uh oh!

turboFei Mar 14, 2024

Choose a reason for hiding this comment

Uh oh!

turboFei Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turboFei Mar 18, 2024

Choose a reason for hiding this comment

Uh oh!

turboFei Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

turboFei Mar 27, 2024

Choose a reason for hiding this comment

Uh oh!

turboFei Mar 27, 2024

Choose a reason for hiding this comment

Uh oh!

RexXiong left a comment

Choose a reason for hiding this comment

Uh oh!

turboFei commented Mar 27, 2024

Uh oh!

RexXiong commented Mar 27, 2024

Uh oh!

RexXiong commented Mar 27, 2024

Uh oh!

turboFei commented Mar 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

turboFei commented Mar 9, 2024 •

edited

Loading

codecov bot commented Mar 9, 2024 •

edited

Loading

turboFei commented Mar 12, 2024 •

edited

Loading

turboFei Mar 18, 2024 •

edited

Loading

turboFei Mar 18, 2024 •

edited

Loading