apache / incubator-uniffle

Uniffle is a high performance, general purpose Remote Shuffle Service.
https://uniffle.apache.org/
Apache License 2.0
387 stars 149 forks source link

[MINOR] improvement(server): Dynamic conf support of server memory watermark #2231

Closed maobaolong closed 2 weeks ago

maobaolong commented 2 weeks ago

What changes were proposed in this pull request?

Support dynamic update the conf of high/low water mark.

Why are the changes needed?

It can be easy to tune the server online without restart leverage this.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Tested Locally.

➜  ~ curl -X POST http://localhost:19948/api/shuffleServer/confOps/update \
-H "Content-Type: application/json" \
-d '{"update":{"rss.server.memory.shuffle.highWaterMark.percentage": "0.5", "rss.server.memory.shuffle.lowWaterMark.percentage": "0.1"}}'

temporarily effective until restart: Update successfully

server.log

[2024-11-03 21:29:20.660] [Jetty-5] [INFO] ConfOpsResource - Dynamic updating ConfVO{update={rss.server.memory.shuffle.highWaterMark.percentage=0.5, rss.server.memory.shuffle.lowWaterMark.percentage=0.1}, delete=[]}
snapshot_b6b93cf5-bf97-4efb-9f20-f106c9278b27
github-actions[bot] commented 2 weeks ago

Test Results

 2 926 files  +31   2 926 suites  +31   6h 11m 19s ⏱️ + 14m 50s  1 088 tests ± 0   1 086 ✅ + 1   2 💤 ±0  0 ❌ ±0  13 630 runs  +60  13 600 ✅ +61  30 💤 ±0  0 ❌ ±0 

Results for commit 88b2b77b. ± Comparison against base commit 680f96a1.

:recycle: This comment has been updated with latest results.

jerqi commented 2 weeks ago

Will the config option be written back to the configuration file?

maobaolong commented 2 weeks ago

@jerqi No, reconfig through restapi is a temporarily way, this can be use when we tuning the configs or in an emergency to avoid restart server.

jerqi commented 2 weeks ago

@jerqi No, reconfig through restapi is a temporarily way, this can be use when we tuning the configs or in an emergency to avoid restart server.

Maybe you should add temporarily key word in the URL.

maobaolong commented 2 weeks ago

curl -X POST http://localhost:19948/api/shuffleServer/confOps/update -> curl -X POST http://localhost:19948/api/shuffleServer/confOps/tempUpdate

Is this ok? @jerqi

jerqi commented 2 weeks ago

curl -X POST http://localhost:19948/api/shuffleServer/confOps/update -> curl -X POST http://localhost:19948/api/shuffleServer/confOps/tempUpdate

Is this ok? @jerqi

curl -X POST http://localhost:19948/api/shuffleServer/confOps/update -> curl -X POST http://localhost:19948/api/shuffleServer/confOps/tempUpdate

Is this ok? @jerqi

http://localhost:19948/api/shuffleserver/confops/temp/update may be better. URI is't case sentive. I prefer not using camel style in the URI.

maobaolong commented 2 weeks ago

@jerqi As this is a use case of rest api update, I start another PR to improve the conf update rest api.

maobaolong commented 2 weeks ago

https://github.com/apache/incubator-uniffle/pull/2235 This is the improvement PR about update conf api

maobaolong commented 2 weeks ago

@jerqi Thanks for your review on this PR, merge it