-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
feat: add config parameters to toggle WAL concurrency and timeouts #21621
Conversation
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.
Nothing else from my end! Interested to see what @lesam thinks as well.
I haven't fully reviewed the plumbing but the overall approach seems fine. It's actually much more extensive than expected but seems like a good move to use contexts for cancellation and propagate it up the API. I was expecting us to also add an explicit WithTimeout to the WAL writing path - does the PR accomplish that? |
I'll double-check if we're already setting any timeouts on the write-path, and add one if not. |
Take()
method on limiter.Fixed
to allow for timeouts & cancellationb553de7
to
53226a5
Compare
Nothing else on my end! |
Closes #21600
I've split the changes into 3 commits to (hopefully) help review:
Take
method, and updates the unit tests forlimiter.Fixed
to show its effectsTake
context.Background
args to all the methods that need it in our tests