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

feat: add config parameters to toggle WAL concurrency and timeouts #21621

Merged
merged 8 commits into from
Jun 9, 2021

Conversation

danxmoran
Copy link
Contributor

Closes #21600

I've split the changes into 3 commits to (hopefully) help review:

  1. Adds the context parameter to the Take method, and updates the unit tests for limiter.Fixed to show its effects
  2. Updates our storage code to plumb a context through to every usage of Take
  3. Adds a bunch of context.Background args to all the methods that need it in our tests

@danxmoran danxmoran requested a review from lesam June 7, 2021 15:58
Copy link
Contributor

@williamhbaker williamhbaker left a 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.

@lesam
Copy link
Contributor

lesam commented Jun 7, 2021

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?

@danxmoran
Copy link
Contributor Author

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.

@danxmoran danxmoran changed the title feat: add context parameter to Take() method on limiter.Fixed to allow for timeouts & cancellation feat: add config parameters to toggle WAL concurrency and timeouts Jun 8, 2021
@danxmoran danxmoran requested a review from williamhbaker June 8, 2021 12:56
@danxmoran danxmoran force-pushed the dm-limiter-timeout-21600 branch from b553de7 to 53226a5 Compare June 8, 2021 12:57
@danxmoran
Copy link
Contributor Author

@lesam @wbaker85 ready for another look

@williamhbaker
Copy link
Contributor

Nothing else on my end!

@danxmoran danxmoran merged commit d747e7e into master Jun 9, 2021
@danxmoran danxmoran deleted the dm-limiter-timeout-21600 branch June 9, 2021 15:03
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.

Update Take in limiter.Fixed to support cancellation/timeouts
3 participants