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.
feat: support for adaptive rate limiting [PIPE-481] #4160
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: support for adaptive rate limiting [PIPE-481] #4160
Changes from 10 commits
e4dffcc
9f62f71
d85cc43
2916ba2
1204bb0
d8302b1
9154c43
5afbf5e
3fd54f2
97d82df
8d469e7
31f553c
181cd0c
4c97fa3
a30df71
0a900a9
7b4cc73
3b1d077
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
There might be a potential data race issue with the
computeLimit
method being called fromCheckLimitReached
andgetLimit
. Ensure that thecomputeLimit
method is thread-safe or consider adding synchronization mechanisms.Also applies to: 50-50
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
computeLimit
method modifiest.config.limit
which could be accessed concurrently, leading to a race condition. Consider adding concurrency control, such as a mutex, to protect this shared state.Committable suggestion
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.
Consider optimizing the
getLimit
method to avoid unnecessary recomputation of the limit if it is already up to date.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
limitSet
field is being accessed and modified without synchronization, which could lead to data races. Ensure that the mutex lock is acquired before settinglimitSet
.Committable suggestion
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.
Before calling
cancel
on the timers, check if it is not nil to avoid a potential nil pointer dereference.Committable suggestion
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 test description "when there is a 429 in the last shortTimeFrequency" could be more descriptive. It should clarify that it expects a reduction in the limit factor due to a 429 response.
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.
There is a typo in the test description; "ins" should be "in".
Committable suggestion
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 state of the adaptive limiter is not reset between test cases, which could lead to the second test case being affected by the first. Consider resetting the state or creating a new instance for each test case to ensure they are independent.
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.
Consider adding error handling or logging when an unknown algorithm type is specified in the configuration, to aid in debugging configuration issues.
default: + log.Warnf("Unknown adaptive algorithm type: %s", name) return &noopAdaptiveAlgorithm{}
Committable suggestion
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
newAdaptiveAlgorithm
function should check if theconfig
parameter is nil before attempting to access its methods to prevent potential nil pointer dereference.Committable suggestion