Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#1651] improvement(netty): Set Netty as the default server type #1653

Closed
wants to merge 7 commits into from

Conversation

rickyma
Copy link
Contributor

@rickyma rickyma commented Apr 17, 2024

What changes were proposed in this pull request?

  1. Set Netty as the default server type.
  2. Update related docs.
  3. Change the default values of configurations rss.server.buffer.capacity.ratio and rss.server.read.buffer.capacity.ratio.
  4. Fix UTs. Some UTs have bugs when testing Netty.

Why are the changes needed?

For #1651.

Also, change the default values of configurations rss.server.buffer.capacity.ratio and rss.server.read.buffer.capacity.ratio, which are more sensible. Because in our tests, we discovered that the metric read_used_buffer_size could potentially only reach up to 3.75GB. The configuration for the shuffle server is: capacity: 140GB, readCapacity: 20GB, and it's under high pressure and high concurrency situations. So I think the default value for rss.server.read.buffer.capacity.ratio is a little bit too high. We can decrease the default value of rss.server.read.buffer.capacity.ratio and increase the default value of rss.server.buffer.capacity.ratio.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Updated UTs.

@rickyma rickyma marked this pull request as draft April 17, 2024 05:23
Copy link

github-actions bot commented Apr 17, 2024

Test Results

 2 384 files  ±0   2 384 suites  ±0   4h 39m 27s ⏱️ + 6m 59s
   919 tests ±0     918 ✅ ±0   1 💤 ±0  0 ❌ ±0 
10 666 runs  ±0  10 652 ✅ ±0  14 💤 ±0  0 ❌ ±0 

Results for commit 545ad8c. ± Comparison against base commit 5ab625b.

♻️ This comment has been updated with latest results.

@rickyma rickyma marked this pull request as ready for review April 17, 2024 08:30
@rickyma rickyma marked this pull request as draft April 17, 2024 08:40
@rickyma rickyma force-pushed the issue-1651 branch 2 times, most recently from b50c76c to 3b348bb Compare April 17, 2024 09:04
@rickyma rickyma marked this pull request as ready for review April 17, 2024 15:42
@rickyma
Copy link
Contributor Author

rickyma commented Apr 17, 2024

@zuston @jerqi PTAL.

@zuston
Copy link
Member

zuston commented Apr 18, 2024

I concern 2 points.

  1. This is a big change for users, and the upgrade is not compatible with version < 0.8 .
  2. Netty will be set as the default only after it can be used on a large scale online.

@rickyma
Copy link
Contributor Author

rickyma commented Apr 18, 2024

We will put the Netty version of Uniffle in production this month. If you are concerned, we can hold off on this PR for now.

@rickyma rickyma mentioned this pull request Apr 19, 2024
3 tasks
@rickyma rickyma closed this May 5, 2024
@rickyma rickyma deleted the issue-1651 branch May 5, 2024 08:33
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.

None yet

2 participants