-
Notifications
You must be signed in to change notification settings - Fork 12
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
Erl call take if no erl config #59
Conversation
@@ -8,6 +8,7 @@ local erl_bucket_size = tonumber(ARGV[7]) | |||
local erl_activation_period_seconds = tonumber(ARGV[8]) | |||
local erl_quota_amount = tonumber(ARGV[9]) | |||
local erl_quota_expiration_epoch = tonumber(ARGV[10]) | |||
local is_erl_enabled = ARGV[11] == "true" and true or false |
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.
just in case, this is the ternary operator in lua, similar to
condition ? true : false
return callback(valError) | ||
let isERLEnabledForBucket = true; | ||
const valERLConfigError = validateConfigIsForElevatedBucket(key, bucketKeyConfig) | ||
if (valERLConfigError) { |
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.
is this semantically different than going down the take()
function at this point?
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.
sorry, i know there was a slack thread on this but I found it hard to parse 😅
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.
I suppose the return values are different?
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.
yes. If we make it go the take()
, we will only be using takeElevated.lua
for buckets with elevated limits configuration and we'll always depend on both scripts.
This way we'll be making sure takeElevated.lua
works for both scenarios and in the future we'll be able to join them in a single method if we wish to do so.
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.
lgtm with minor suggestion 👍
Co-authored-by: LeweyM <36539330+LeweyM@users.noreply.github.com>
Description
This PR includes 3 minor fixes:
takeElevated
for a bucket with no elevated configuration, it allows the call to takeElevated.lua but indicating ERL is disabled so it doesn't attempt to activate it. ItTesting
Checklist