-
Notifications
You must be signed in to change notification settings - Fork 2.3k
segment replication and merged segment warmer use a dedicated threadpool #18769
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
base: main
Are you sure you want to change the base?
segment replication and merged segment warmer use a dedicated threadpool #18769
Conversation
|
❌ Gradle check result for 934f8ba: null Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: guojialiang <[email protected]>
934f8ba to
70888c2
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18769 +/- ##
============================================
- Coverage 72.86% 72.74% -0.12%
+ Complexity 68571 68482 -89
============================================
Files 5566 5566
Lines 314513 314673 +160
Branches 45636 45652 +16
============================================
- Hits 229167 228912 -255
- Misses 66789 67183 +394
- Partials 18557 18578 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi, @Bukhtawar @ashking94 |
ashking94
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing on this. I wanted to understand on what is the motivation for this change? Did you see any issue which led to you to do this?
| final int halfProcMaxAt5 = halfAllocatedProcessorsMaxFive(allocatedProcessors); | ||
| final int halfProcMaxAt10 = halfAllocatedProcessorsMaxTen(allocatedProcessors); | ||
| final int genericThreadPoolMax = boundedBy(4 * allocatedProcessors, 128, 512); | ||
| final int replicationThreadPoolMax = boundedBy(4 * allocatedProcessors, 128, 512); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How have we arrived at this number?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How have we arrived at this number?
To better manage PR, no other changes were introduced except for a dedicated replication thread pool.
The thread pool size remains consistent with the generic thread pool.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am assuming recovery will still continue to use the generic threadpool?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Hi, @ashking94, thanks for review the PR.
When the number of shards on a node is large (say As segment replication becomes increasingly important, many new ideas are based on segment replication, such as: merged segment warmer, Hybrid block level fetch and Adaptive Refresh. By introducing a dedicated thread pool, we can more precisely control tasks of the data replication, and there are no obvious drawbacks. |
|
This PR is stalled because it has been open for 30 days with no activity. |
|
This PR is stalled because it has been open for 30 days with no activity. |
Description
Add an dedicated
REPLICATIONthread pool.Related Issues
Resolves #[18755]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.