fix(redisquota): use dimensions in quota key to match memquota #16022
Conversation
istio-testing
added
the
do-not-merge/work-in-progress
googlebot
added
the
cla: yes
istio-testing
added
the
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: douglas-reid The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
douglas-reid
requested review from
bianpengyuan,
costinm,
geeknoid,
kyessenov,
nmittler and
ozevren
as code owners
istio-testing
removed
the
do-not-merge/work-in-progress
| for k2 := range v { | ||
| mk = append(mk, k2) | ||
| } | ||
| sort.Strings(mk) |
There was a problem hiding this comment.
this could also be sorted at init time.
There was a problem hiding this comment.
yup. this file was just a copy/paste job from memquota package to ensure that it was being done the same way.
|
|
||
| // ensure stable order | ||
| for k := range labels { | ||
| keys = append(keys, k) |
There was a problem hiding this comment.
nit: This could be moved to init time.
Could be done in a follow up.
| sort.Strings(mk) | ||
|
|
||
| for _, k2 := range mk { | ||
| buf.WriteString(k2) // nolint: gas |
There was a problem hiding this comment.
This doesn't sound right, "a=bc" is same as "ab=c". I know this is the same bug in the original code. Can you at least add a comment where this code is copied from so that we fix both when needed?
|
In response to a cherrypick label: new pull request created: #16106 |
This PR is meant to address the issues raised in #15543 .
It duplicates the keys.go into the
redisquotapackage (for simplicity) and updates the key definition to usemakeKey.[ ] Configuration Infrastructure
[ ] Docs
[ ] Installation
[ ] Networking
[ ] Performance and Scalability
[ X ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure