Global ratelimiter helper: a small atomic-like map #6027
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Global ratelimiter helper: an atomic-like map to simplify other use This commit largely exists to break this out for separate review, as it's relatively simple and isolated.
Groxx
requested review from
Shaddoll,
neil-xie,
davidporter-id-au,
agautam478,
jakobht,
3vilhamster,
sankari165,
dkrotx,
taylanisikdemir and
demirkayaender
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Groxx
changed the title
Groxx
changed the title
Shaddoll
reviewed
May 21, 2024
Shaddoll
reviewed
May 21, 2024
common/quotas/global/collection/internal/atomicmap_external_test.go
Outdated
Show resolved
Hide resolved
Shaddoll
approved these changes
May 21, 2024
Groxx
commented
May 21, 2024
common/quotas/global/collection/internal/atomicmap_external_test.go
Outdated
Show resolved
Hide resolved
timl3136
pushed a commit
to timl3136/cadence
that referenced
this pull request
Jun 6, 2024
This commit largely exists to break this out for separate review, as it's relatively simple and isolated. In a later PR, this will be used by the "limiter"-side logic to hold known ratelimiters, and simplify its logic. So it does just two core things, and exposes a *very* small API that's intended to be difficult or impossible to misuse: 1. Tracks length, so a "iterate over everything and collect data for sending to aggregators" loop can pre-allocate a collection that is likely large enough to store everything. 2. Implicitly initializes missing values, so the using logic does not ever need to check for existence or explicitly handle fallbacks or decide how to order atomic operations. It could have other methods, but so far they do not seem necessary. Delete is not currently planned to be used, but it seems important to reserve / guarantee it's possible to build, so I've included that as well. --- This is not intended to be broadly reusable as we do not currently have any other locations that will use it, and the auto-initializing behavior is potentially unique. It's mostly just isolating and simplifying some logic to prevent accidental misuse, because early iterations had some easily-missed flaws. We very likely *should* have a "just a type-safe `sync.Map`" wrapper, which this could easily be changed to use, but the vast majority of that API would be unused here so I haven't built that.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit largely exists to break this out for separate review, as it's relatively simple and isolated.
In a later PR, this will be used by the "limiter"-side logic to hold known ratelimiters, and simplify its logic. So it does just two core things, and exposes a very small API that's intended to be difficult or impossible to misuse:
It could have other methods, but so far they do not seem necessary. Delete is not currently planned to be used, but it seems important to reserve / guarantee it's possible to build, so I've included that as well.
This is not intended to be broadly reusable as we do not currently have any other locations that will use it, and the auto-initializing behavior is potentially unique. It's mostly just isolating and simplifying some logic to prevent accidental misuse, because early iterations had some easily-missed flaws.
We very likely should have a "just a type-safe
sync.Map" wrapper, which this could easily be changed to use, but the vast majority of that API would be unused here so I haven't built that.