Skip to content
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

KIP-546 Client quota APIs #1909

Merged
merged 2 commits into from
Sep 13, 2021
Merged

KIP-546 Client quota APIs #1909

merged 2 commits into from
Sep 13, 2021

Conversation

agriffaut
Copy link
Contributor

@agriffaut agriffaut requested a review from bai as a code owner March 29, 2021 10:28
@agriffaut agriffaut changed the title Client quotas api KIP-546 Client quota APIs Mar 30, 2021
@dnwe
Copy link
Collaborator

dnwe commented Apr 27, 2021

@agriffaut thanks for your contribution, this is great!

One question I had after an initial scan over the PR, was there a design goal around consistently using slices of pointers in the protocol definitions here? The slices will always either be empty or filled and will never have nil values, so I'm not sure why we wouldn't make these slices of the concrete type? As we (currently) have a strong coupling between the parameters for the high level functions in admin.go and the protocol structs, we similarly end up with map[string]*string etc.

@dnwe
Copy link
Collaborator

dnwe commented May 5, 2021

I rebased and it also looks like TestDescribeClientQuotasResponse might be flakey. Please could you take a look?

2021-05-05T22:41:43.7835979Z === RUN   TestDescribeClientQuotasResponse
2021-05-05T22:41:43.7836889Z     request_test.go:96: Encoding Multi Value failed
2021-05-05T22:41:43.7837592Z         got  [0 0 0 0 0 0 255 255 0 0 0 1 0 0 0 1 0 4 117 115 101 114 255 255 0 0 0 2 0 18 99 111 110 115 117 109 101 114 95 98 121 116 101 95 114 97 116 101 65 46 132 128 0 0 0 0 0 18 112 114 111 100 117 99 101 114 95 98 121 116 101 95 114 97 116 101 65 46 132 128 0 0 0 0] 
2021-05-05T22:41:43.7838308Z         want [0 0 0 0 0 0 255 255 0 0 0 1 0 0 0 1 0 4 117 115 101 114 255 255 0 0 0 2 0 18 112 114 111 100 117 99 101 114 95 98 121 116 101 95 114 97 116 101 65 46 132 128 0 0 0 0 0 18 99 111 110 115 117 109 101 114 95 98 121 116 101 95 114 97 116 101 65 46 132 128 0 0 0 0]
2021-05-05T22:41:43.7839480Z --- FAIL: TestDescribeClientQuotasResponse (0.00s)

@dnwe
Copy link
Collaborator

dnwe commented Jun 2, 2021

@agriffaut 👋 any thoughts on the above comments?

@agriffaut
Copy link
Contributor Author

@dnwe I finally managed to work again on that PR, sorry for the delay.
As you said, some pointers were useless and have been removed !

Please tell me if this looks good to you!

Copy link
Contributor

@bai bai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bai
Copy link
Contributor

bai commented Sep 7, 2021

Thanks for your contribution @agriffaut.

I'll wait for dnwe to review before we ship this 🙏🏼

@bai bai requested a review from dnwe September 13, 2021 03:28
Copy link
Collaborator

@dnwe dnwe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks again for the PR and the updates 🙇

@bai
Copy link
Contributor

bai commented Sep 13, 2021

Thanks @dnwe for reviewing 🙏🏼

@bai bai merged commit a37f45d into IBM:master Sep 13, 2021
dnwe pushed a commit that referenced this pull request Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants