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.

Sign up for GitHub

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

Jump to bottom

Change client default value of qps and burst to constant #26756

Merged
merged 2 commits into from
Jun 30, 2016
Merged

Change client default value of qps and burst to constant #26756

merged 2 commits into from
Jun 30, 2016

Conversation

hongchaodeng
Copy link
Contributor

No description provided.

@hongchaodeng
Copy link
Contributor Author

@caesarxuchao @lavalamp @xiang90

@lavalamp
Copy link
Member

lavalamp commented Jun 3, 2016

Define in constants? Otherwise I expect the comments will age and become misleading.

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Jun 3, 2016
@hongchaodeng
Copy link
Contributor Author

What's this code:

kubernetes/cmd/libs/go2idl/client-gen/generators/generator_for_group.go

Lines 218 to 223 in f32f396

if config.QPS == 0 {
config.QPS = 5
}
if config.Burst == 0 {
config.Burst = 10
}

Do I need run some auto-gen program? Or do I just change it to some constant there?

@lavalamp @caesarxuchao

@caesarxuchao
Copy link
Member

The code you linked to is template used by generator. You can change it to using a constant and then run hack/update-codegen.sh to update the generated clientsets.

@hongchaodeng hongchaodeng changed the title docs: client default config of qps and burst Change client default value of qps and burst to constant Jun 3, 2016
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 3, 2016
@xiang90
Copy link
Contributor

xiang90 commented Jun 3, 2016

looks good.

@xiang90 xiang90 added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jun 3, 2016
@caesarxuchao
Copy link
Member

Looks good. As a side note, we should differentiate constants from variables in go2idl.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 3, 2016
@hongchaodeng
Copy link
Contributor Author

@lavalamp @caesarxuchao
All tests passed. Are we making this into v1.3? If not, what schedule shall we expect?

@caesarxuchao
Copy link
Member

I don't think this is a v1.3 issue. We can apply the lgtm but it won't be merged until the code freeze ends. Seems it's Jun 20 (https://github.com/kubernetes/kubernetes/wiki/Release-1.3#jun-20---jun-24).

lavalamp
lavalamp reviewed Jun 4, 2016
View reviewed changes
pkg/client/typed/dynamic/client.go
}
if conf.Burst == 0 {
conf.Burst = 10
conf.Burst = restclient.DefaultBurst
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this? Could we add this to RESTClientFor (if it doesn't already) and avoid putting this logic in every single generated client?

@hongchaodeng hongchaodeng force-pushed the cli branch 2 times, most recently from ee1ffa4 to 485af0b Compare June 6, 2016 23:18
@k8s-bot
Copy link

k8s-bot commented Jun 7, 2016

GCE e2e build/test passed for commit 485af0b56cbaac7e3daf57251cfe3616054eb421.

@hongchaodeng
Copy link
Contributor Author

@lavalamp
Addressed your comments and all tests passed.

@hongchaodeng
Copy link
Contributor Author

@caesarxuchao Can you review this?

@lavalamp
Copy link
Member

lavalamp commented Jun 7, 2016

Sorry, we're really busy on 1.3 at the moment-- we'll cut the branch at the
end of the week and then this will get a timeslice.

On Tue, Jun 7, 2016 at 9:20 AM, Hongchao Deng notifications@github.com
wrote:

@caesarxuchao https://github.com/caesarxuchao Can you review this?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#26756 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAngloGFpAeZUTk7csFLWFfuRqbDAPEDks5qJZoxgaJpZM4ItGvd
.

@hongchaodeng
Copy link
Contributor Author

@caesarxuchao
All comments have been addressed. Can you review it?

@caesarxuchao caesarxuchao self-assigned this Jun 27, 2016
caesarxuchao
caesarxuchao reviewed Jun 27, 2016
View reviewed changes
pkg/client/restclient/config.go
@@ -158,6 +163,12 @@ func RESTClientFor(config *Config) (*RESTClient, error) {
if config.NegotiatedSerializer == nil {
return nil, fmt.Errorf("NegotiatedSerializer is required when initializing a RESTClient")
}
if config.QPS == 0.0 {
Copy link
Member

@caesarxuchao caesarxuchao Jun 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I would suggest that not to modify the config. Caller of RESTClientFor doesn't expect to get its config changed.

It seems the only place that uses config.QPS and config.Burst is NewRESTClient, could you copy them to two local variables and pass them to NewRESTClient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what @lavalamp suggested. Could you two coordinate on final decision and ping again? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

could you copy them to two local variables and pass them to NewRESTClient?

This doesn't conflict with my recommendation to put this defaulting logic here. :) Not modifying config seems good.

caesarxuchao
caesarxuchao reviewed Jun 28, 2016
View reviewed changes
pkg/client/restclient/config.go
@@ -93,10 +98,10 @@ type Config struct {
// on top of the returned RoundTripper.
WrapTransport func(rt http.RoundTripper) http.RoundTripper

// QPS indicates the maximum QPS to the master from this client. If zero, QPS is unlimited.
// QPS indicates the maximum QPS to the master from this client. If zero, it will be set to DefaultQPS: 5.
Copy link
Member

Choose a reason for hiding this comment

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

nit: "it will be set to DefaultQPS: 5" -> "the created RESTClient will use DefaultQPS: 5". The config doesn't get changed.

@caesarxuchao
Copy link
Member

LGTM. Just two nits regarding the comments. Fix them if you can get to them before merge.

@caesarxuchao caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 28, 2016
@hongchaodeng
Copy link
Contributor Author

Addressed. Thanks for the review.

@caesarxuchao caesarxuchao added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 28, 2016
@caesarxuchao
Copy link
Member

Thanks for contributing!

@k8s-bot
Copy link

k8s-bot commented Jun 29, 2016

GCE e2e build/test passed for commit 56fb1688ac5f49186e4083f5080fa1763c0f8cfc.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2016
@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 29, 2016
@k8s-bot
Copy link

k8s-bot commented Jun 29, 2016

GCE e2e build/test passed for commit 55d3597.

@caesarxuchao caesarxuchao added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 29, 2016
@k8s-bot
Copy link

k8s-bot commented Jun 29, 2016

GCE e2e build/test passed for commit 55d3597.

@hongchaodeng
Copy link
Contributor Author

hongchaodeng commented Jun 29, 2016 •
edited
Loading

@caesarxuchao
Just figure out one potential problem.
If the config.{QPS, Burst} didn't get changed, it won't be the same as previous where the fields in config were changed.

@caesarxuchao
Copy link
Member

I think the config is always supplied to RESTClientFor so there should be no problem. Do you see any counter example?

@hongchaodeng
Copy link
Contributor Author

@k8s-bot node e2e test this issue: #IGNORE

@k8s-bot
Copy link

k8s-bot commented Jun 29, 2016

GCE e2e build/test failed for commit 55d3597.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@ixdy
Copy link
Member

ixdy commented Jun 29, 2016

@k8s-bot test this please, issue #IGNORE

(Sorry, temporarily broke Jenkins.)

@k8s-bot
Copy link

k8s-bot commented Jun 29, 2016

GCE e2e build/test passed for commit 55d3597.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Jun 30, 2016

GCE e2e build/test passed for commit 55d3597.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit d8d5ab2 into kubernetes:master Jun 30, 2016
@hongchaodeng hongchaodeng deleted the cli branch June 30, 2016 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lavalamp lavalamp

@caesarxuchao caesarxuchao

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@hongchaodeng @lavalamp @caesarxuchao @xiang90 @k8s-bot @ixdy @k8s-github-robot @googlebot