-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
vtorc
: use golang.org/x/sync/semaphore
, add flag for db concurrency
#17837
base: main
Are you sure you want to change the base?
vtorc
: use golang.org/x/sync/semaphore
, add flag for db concurrency
#17837
Conversation
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
vtorc
: use golang.org/x/sync/semaphore, add flag for db concurrencyvtorc
: use golang.org/x/sync/semaphore
, add flag for db concurrency
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17837 +/- ##
==========================================
- Coverage 67.45% 67.44% -0.01%
==========================================
Files 1592 1592
Lines 258167 258227 +60
==========================================
+ Hits 174143 174165 +22
- Misses 84024 84062 +38 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Tim Vaillancourt <tim@timvaillancourt.com>
0fbc78e
to
0edc405
Compare
viperutil.Options[int64]{ | ||
FlagName: "backend-read-concurrency", | ||
Default: 32, | ||
Dynamic: true, |
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.
Rest is all good, but I don't think this field should be dynamic. Since we initialize the semaphore with value we read for this config once, even if the users change it later, the change won't be affected.
ctx, cancel := context.WithTimeout(context.Background(), maxBackendOpTime) | ||
defer cancel() | ||
|
||
if err := instanceWriteSem.Acquire(ctx, 1); err != nil { | ||
return err |
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.
We didn't have a test before, but maybe its worth adding one to check that these concurrency controls are respected.
Description
This PR migrates the concurrency control for VTOrc's backend to use
golang.org/x/sync/semaphore
and a flag is added to control the limitRelated Issue(s)
#17330
Checklist
Deployment Notes