-
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
Ensure all topo read calls consider --topo_read_concurrency
#17276
Ensure all topo read calls consider --topo_read_concurrency
#17276
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
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #17276 +/- ##
==========================================
+ Coverage 67.51% 67.53% +0.02%
==========================================
Files 1580 1581 +1
Lines 253805 253945 +140
==========================================
+ Hits 171358 171506 +148
+ Misses 82447 82439 -8 ☔ View full report in Codecov by Sentry. |
@@ -181,6 +186,9 @@ var ( | |||
|
|||
FlagBinaries = []string{"vttablet", "vtctl", "vtctld", "vtcombo", "vtgate", | |||
"vtorc", "vtbackup"} | |||
|
|||
// Default read concurrency to use in order to avoid overhwelming the topo server. | |||
DefaultReadConcurrency int64 = 32 |
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.
Moved + renamed from DefaultConcurrency
from keyspace.go
DefaultReadConcurrency
seems more accurate as this only applies to reads
--topo_read_concurrency
--topo_read_concurrency
10ad8a6
to
7d318cf
Compare
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.
The only concern I have is what I said on the linked issue #17275 (comment)
Otherwise the implementation looks fine to me.
@deepthi 👋, makes sense I'll re-work this so all cells (global or local) get a limit equal to |
@deepthi updated to use a limit per global and local cell. I also added this a minor change to the v22 changelog 👍 |
Signed-off-by: Tim Vaillancourt <[email protected]>
df7655d
to
2f1e599
Compare
Signed-off-by: Tim Vaillancourt <[email protected]>
👋 @deepthi I think I'm done tweaking this PR now We've backported this to our v19 branch and tested it in a low-volume environment - no issues noticed. Testing on a prod-like environment is to come if we'd like to wait for that |
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.
Nice work on this @timvaillancourt !
The only non-nit that I had was related to the function timing stats. Let me know what you think. I can come back to this quickly and we can get it merged.
Thanks!
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.
LGTM apart from the outstanding comments
Signed-off-by: Tim Vaillancourt <[email protected]>
@mattlord thanks for the review. Responded to some comments and addressed @GuptaManan100's point I think the outstanding topic is which approach to the operation vs. semaphore-wait stats timings. Thoughts appreciated 🙇 |
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.
Using request changes just to be sure that we resolve the stats questions before merging.
Signed-off-by: Tim Vaillancourt <[email protected]>
@mattlord no problem! Yeah with all those
I agree, this is some important code. Adding some tests |
Signed-off-by: Tim Vaillancourt <[email protected]>
Description
This PR implements the feature request #17275 by ensuring that all topo read calls respect the
--topo_read_concurrency
flag, specificallyGet
,GetVersion
,List
,ListDir
operationsAs discussed in #17275, there is a separate semaphore/limit for "global" and "cell" reads, ie: by default there can be up to 32 concurrent "global" topo reads and 32 concurrent reads per "local" cell
Related Issue(s)
--topo_read_concurrency
#17275Checklist