KIP-546: Add Client Quota APIs #1119
Conversation
| entries := make([]alterclientquotas.Entry, len(req.Entries)) | ||
|
|
||
| for entryIdx, entry := range req.Entries { | ||
| entities := make([]alterclientquotas.Entity, len(entry.Entities)) |
There was a problem hiding this comment.
I followed the pattern used elsewhere where we choose not to reuse the types defined in the protocol package and create new types in the kafka package
alterclientquotas.go
Outdated
| } | ||
|
|
||
| responseEntries[responseEntryIdx] = AlterClientQuotaResponseQuotas{ | ||
| ErrorCode: responseEntry.ErrorCode, |
There was a problem hiding this comment.
It looked like elsewhere we created a new errors map field in the return and grouped all the errors there instead of keeping them nested under the individual entries. Do we want that instead of what I have here?
petedannemann
changed the title
| // The quota configuration value to set, otherwise ignored if the value is to be removed. | ||
| Value float64 | ||
|
|
||
| // Whether the quota configuration value should be removed, otherwise set. |
There was a problem hiding this comment.
otherwise set.
Is there a word missing at the end there ("false" maybe)?
There was a problem hiding this comment.
Most of these doc strings (including this one) are just quotes from the API docs. This verbiage seems common in the Kafka API docs so I think it is ok. I can change it if you feel otherwise
alterclientquotas_test.go
Outdated
| client, shutdown := newLocalClient() | ||
| defer shutdown() | ||
|
|
||
| expectedAlterResp := AlterClientQuotasResponse{ |
There was a problem hiding this comment.
Maybe it's the standard practice to define the expected value early but it feels more logical to me just before the assert (and after the err check)
There was a problem hiding this comment.
You're right, I just made this change.
| type Component struct { | ||
| EntityType string `kafka:"min=v0,max=v1"` | ||
| MatchType int8 `kafka:"min=v0,max=v1"` | ||
| Match string `kafka:"min=v0,max=v1,nullable"` |
There was a problem hiding this comment.
It looks like v1 is compact and nullable.
|
Thanks for the review @rhansen2! I believe I addressed your comments, let me know if there are fixes needed or other changes required! |
| type Entity struct { | ||
| EntityType string `kafka:"min=v0,max=v0|min=v1,max=v1,compact"` | ||
| EntityName string `kafka:"min=v0,max=v0,nullable|min=v1,max=v1,nullable,compact"` | ||
| } | ||
|
|
||
| type Value struct { | ||
| Key string `kafka:"min=v0,max=v0|min=v1,max=v1,compact"` | ||
| Value float64 `kafka:"min=v0,max=v1"` | ||
| } | ||
|
|
||
| type ResponseQuotas struct { | ||
| Entities []Entity `kafka:"min=v0,max=v1"` | ||
| Values []Value `kafka:"min=v0,max=v1"` | ||
| } |
There was a problem hiding this comment.
I believe these types also need to be marked with a tag field for v1.
|
@rhansen2 thanks for the review! I added tags to those types as well as similar ones in alterclientquotas |
KIP: https://cwiki.apache.org/confluence/display/KAFKA/KIP-546%3A+Add+Client+Quota+APIs+to+the+Admin+Client
API docs:
https://kafka.apache.org/protocol#The_Messages_DescribeClientQuotas
https://kafka.apache.org/protocol#The_Messages_AlterClientQuotas
I followed a similar code style to what was used for alterconfigs. Let me know if there is a better code style to use for this.
If this PR is too large to review I could split it into three pieces: