client request metrics should be registered only once #17885
Conversation
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
1 similar comment
|
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
|
Labelling this PR as size/XS |
k8s-github-robot
added
the
size/XS
|
Yikes. |
|
ok to test |
smarterclayton
added
the
lgtm
|
GCE e2e test build/test passed for commit 0521481. |
|
LGTM - thanks! |
|
The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label? |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e test build/test passed for commit 0521481. |
|
Automatic merge from submit-queue |
Auto commit by PR queue bot
| @@ -48,6 +48,10 @@ import ( | |||
| // are therefore not allowed to set manually. | |||
| var specialParams = sets.NewString("timeout") | |||
|
|
|||
| func init() { | |||
| metrics.Register() | |||
There was a problem hiding this comment.
This is an improvement, but metrics itself should protect against double-registration IMO.
There was a problem hiding this comment.
@lavalamp It does protect you from this. And in the k8s metrics pkg, it has a double protection by using sync.Once. But this change makes things easier to reason about.
There was a problem hiding this comment.
Ah, great!
On Mon, Nov 30, 2015 at 1:02 PM, Xiang Li notifications@github.com wrote:
In pkg/client/unversioned/request.go
#17885 (comment)
:@@ -48,6 +48,10 @@ import (
// are therefore not allowed to set manually.
var specialParams = sets.NewString("timeout")+func init() {
- metrics.Register()
@lavalamp https://github.com/lavalamp It does protect you from this.
And in the k8s metrics pkg, it has a double protection by using sync.Once.
But this change makes things easier to reason about.—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/17885/files#r46203052.
There was a problem hiding this comment.
Yeah - the code before was correct - it wasn't correctness improvement.
It is just that we avoid some unnecessary checks (under lock) when creating every single request.
This is one of the contention found in density tests.
@wojtek-t @gmarek @fgrzadkowski