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.
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
Global ratelimiter: everything else #6141
Global ratelimiter: everything else #6141
Changes from 26 commits
573ea38
8100d32
794f664
4f1cccc
fae6457
b6df0e5
9eb9ca8
1d7afcf
84b5167
e7750ad
f472646
2a3b361
2b1b78e
816f3e5
9361aa1
8c0d9c1
9e892fd
62c4c9b
5c58beb
7403ba9
135fdc7
789edf6
943b935
7618574
f631296
0942c4c
02cff69
1f37531
0d7d4cb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
you are not checking existence of
yarpc.WithShardKey()
when len > 0There 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 there isn't a good way to do so, yes.
(see the comment lines immediately below)
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.
WithShardKey is a CallOption: https://github.com/yarpc/yarpc-go/blob/v1.73.0/call.go#L70
Which is an encoding.CallOption: https://github.com/yarpc/yarpc-go/blob/v1.73.0/call.go#L34
Which is this struct: https://github.com/yarpc/yarpc-go/blob/dev/api/encoding/call_option.go#L29-L31
Which is implemented by this here: https://github.com/yarpc/yarpc-go/blob/dev/api/encoding/call_option.go#L60-L70
So our only real option (AFAICT) is to reflect on the internal
opt
field, and try to read the un-typedfield.String()
raw contents, and hope it's recognizable as a shard key (because reflection will not give us the actual type on private fields).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.
poking a bit more, I think this works:
and we should be able to detect it changing in tests, but tbh I'm not sure how reliable non-panicky that'd be with other args / future yarpc changes 🤔.
a "recover and log and fail" might take care of the edge cases we care about well enough, just feels like a potential bit of pain in a surprise-future.
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.
how about making shard key an explicit parameter?
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 think this might end up being the nicest route... but doing that means changing quite a lot of code, as well as a few pieces of codegen :\
I'm happy to stick it on a "try it soon" to-do list (there are a few followups worth doing), but tbh it doesn't feel worth making this PR even bigger. There's only one caller, and that's likely all there ever will be.
(it'll also be a pretty clear / self-contained followup, as opposed to mingling in a few places in this PR)
I did try a different route that @taylanisikdemir suggested (pushing the key-arg into the request object), and while I kinda liked that better than the CallOption slice... it ran into quite a few issues when I fully built it :\ both import cycles (for nicer types) and "this really just moves the flaw, it doesn't eliminate it, and it's more complicated"-like issues.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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'd add debug logs to a few places in this func for the initial rollout
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.
+1, I'd not bother maybe as part of this PR, but at allowing hostnames to be mapped to a particular rate-limit (via sampling or debug logs) would probably be operationally be quite helpful in diagnosing any problems
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.
since I broadly agree but I think there are a few overlapping uses:
what kind of goals do you think are worth building immediately vs "soon"?
OTOH I think there are:
and... feels like maybe other scenarios are useful? I haven't been involved in much of our ring issues, not sure what worked / would have helped in those.
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.
nit: more readable version with bool ok
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.
tbh I don't think I'd label that "more readable" - it adds a var and allows a third category of behavior, where "current" exists but is empty.
len(current)==0
has only two cases to consider: empty or not.though I suppose empty is fine to append to. just feels odd/worse to me to imply "if something else added here, use it verbatim", particularly with how weird-mutation-prone slices are.
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 added as a nit comment but since you are also interested in this topic let me share more on my reasoning. (not a blocker for this PR by the way)
Both options achieve the same thing for this case. The only advantage of using
if !ok
approach is the consistency across various map existence check scenarios in the codebase. I guess we'd agree that when a convention is consistently applied in the codebase it helps with reading experience. If each developer chooses a viable option for their own taste then you lose some readability score.Bool ok is part of the map interface and handles all cases. However using the values to determine existence fall short if the map can contain values that are the default values such as integer 0 or a zero struct value. In that case checking
if myMap[key] != 0
doesn't work. This means you would have to useif !ok
for some maps and zero value check for others. Obviously inconsistent => less readable.We can argue how many milliseconds does this add to reading experience :) From principles perspective, I see the obvious choice of sticking with the approach that works for all cases to stay consistent.
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.
tbh I don't think we're particularly consistent about this in the codebase. but for this, zero values are valid and safe and comparable in this case, which is why I don't differentiate between "exists" and "not exists". if they were unsafe or ambiguous I'd absolutely agree,
ok
is the only reasonable choice.I do wish Go had something like python's
defaultdict(constructor)
. it makes more-efficient-init stuff like this a lot more readable, particularly if there are multiple branches that need the same checks :\Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.