-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Handling multiple rate-limits #382
Conversation
319dcb5
to
c36ea77
Compare
Sounds good 👍. Except for the manual |
However I am not sure about supporting the old format. Because now we are either forever supporting a deprecated format, or just fighting off an inevitable deprecation. We could drop it now, while we are still not in 1.0, and optionally provide a migration script and instructions on how to run it. Instead of dropping it later on when old providers will have both formats in their plugins configuration (the old and the new one) and it will be harder to migrate. |
I thought of it, and I was about to implement it. Then I thought more carefully and I found the following reasons to postpone it:
Thoughts? |
return false, DaoError(err, constants.DATABASE_ERROR_TYPES.SCHEMA) | ||
end | ||
end | ||
|
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.
@thibaultcha to fix the problem I had to implement this check. Basically this is the problem:
- BaseDao invokes the
validate_entity
. The entity is theplugin_configuration
, which has aself_check
. - The
self_check
is never being invoked on the underlying plugin's schema. - This code invokes the underlying plugins's
self_check
if it exists.
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.
Sounds good, but you are locking the self_check to a SCHEMA error, and it shouldn't be. It can be any other error type too. You should return the error as it is returned by self_check
, it is up to the plugin to decide what error it is exactly. Maybe the error comes from Cassandra being unaccessible for example.
Otherwise the check sounds good to me. A better solution would be to investigate on why it is not invoked from validate_entity
, because the function calls itself on sub-schemas so if there is no other error, self_check
should be recursively called already.
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.
because the function calls itself on sub-schemas
I believe this part is missing.
89def70
to
7a6a5e8
Compare
62e8bd7
to
55352bc
Compare
Handling multiple rate-limits
Sorry to comment on a closed PR, but do we have the requested benchmarks when a API is configured with multiple rate limits? Just curious! |
Handling multiple rate-limits Former-commit-id: d49df90d93864c9b6aca6018f6735e4f0ebfa4a6
Closes issue #205 and deprecates #224.
This pull requests enables support for multiple rate-limit periods on the same API.
New plugin configuration value format
This PR promotes a new format for setting up the
ratelimiting
plugin, whose value is nowvalue.{period}={limit}
, for example:value.second=10
instead of the previous onevalue.limit=10&value.period=second
.The plugin will transparently handle the old value format without requiring a migration of the data.