Avoid unnecessary decoding in etcd3 client #34435
Conversation
wojtek-t
added
do-not-merge
k8s-github-robot
added
the
size/M
|
Jenkins GCE Node e2e failed for commit 7f63c5d. Full PR test history. The magic incantation to run this job again is |
| @@ -52,6 +52,7 @@ type watchChan struct { | |||
| initialRev int64 | |||
| recursive bool | |||
| filter storage.FilterFunc | |||
| filterAcceptsAll bool | |||
There was a problem hiding this comment.
do we need this additional field? can we simply set filter to nil if we accept everything?
There was a problem hiding this comment.
basically (wc *watchChan) acceptAll bool (return wc.filter == nil)
There was a problem hiding this comment.
Well - that would mean that whenever we are accessing wc.filter, we need to check whether it is nil or not. This will complicate the code in my opinion.
There was a problem hiding this comment.
@wojtek-t transform is not the only place we access it? if not, i agree.
There was a problem hiding this comment.
transform is the only place, but there are 4 places in transform...
There was a problem hiding this comment.
This might be overkill, but still want to bring it up.
we can add another layer of abstraction to get rid of the duplicated field.
wc.filter() {
if wc.internalFilter == nil{
...
}
wc.internalFilter()
}
wc.acceptAll {return wc.internalFilter == nil}this is totally up to you though. not blocking issue.
There was a problem hiding this comment.
OK - if you think it's worth it, I can change that tomorrow.
| res = &watch.Event{ | ||
| Type: watch.Modified, | ||
| Object: curObj, | ||
| } | ||
| case curObjPasses && !oldObjPasses: |
There was a problem hiding this comment.
i feel we have too many indentations here. can we return directly in this case and remove the else at line 267?
|
The idea looks good to me. @hongchaodeng Can you also take a look? |
wojtek-t
force-pushed
the
avoid_unnecessary_decoding
branch
from
7f63c5d to
fe706bb
Compare
|
Jenkins GCI GCE e2e failed for commit fe706bb0d9f1ab0923ef459e98bc2341a96fcf15. Full PR test history. The magic incantation to run this job again is |
|
The approach lgtm. |
k8s-github-robot
added
the
needs-rebase
wojtek-t
force-pushed
the
avoid_unnecessary_decoding
branch
from
fe706bb to
b675b22
Compare
|
@xiang90 @hongchaodeng - comments applied. PTAL |
wojtek-t
removed
the
do-not-merge
k8s-github-robot
removed
the
needs-rebase
|
Jenkins Kubemark GCE e2e failed for commit b675b22. Full PR test history. The magic incantation to run this job again is |
|
@k8s-bot gce e2e test this, issue: #IGNORE (firewall quota) |
|
@k8s-bot kubemark test this, issue: #IGNORE (gcloud auth issues) |
|
@k8s-bot gce e2e test this, issue: #IGNORE (firewall quota) |
1 similar comment
|
@k8s-bot gce e2e test this, issue: #IGNORE (firewall quota) |
|
/lgtm |
k8s-github-robot
added
the
lgtm
|
@k8s-bot gce e2e test this, issue: #IGNORE (previous run didn't start) |
|
Jenkins GCE e2e failed for commit b675b22. Full PR test history. The magic incantation to run this job again is |
wojtek-t
added
the
priority/backlog
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue |
|
Removing label |
jessfraz
added
cherry-pick-approved
#31704-#32831-#32907-#33003-#33349-#31190-#33581-#34089-#34234-#32822-#33393-#34246-#34435-#32477-upstream-release-1.4 Automatic merge from submit-queue Automated cherry pick of #31189 #31704 #32831 #32907 #33003 #33349 #31190 #33581 #34089 #34234 #32822 #33393 #34246 #34435 #32477 upstream release 1.4 We are going to release etcd v3 in 1.4.x release. ``` Cherrypick etcd v3-related bug fixes to 1.4 branch ``` Those PRs include: - Updates of etcd Godeps - Update to pkg/storage/etcd directory - Two PR that were unnecessary to avoid conflicts: #31189 #31704 Automated cherry pick of #31189 #31704 #32831 #32907 #33003 #33349 #31190 #33581 #34089 #34234 #32822 #33393 #34246 #34435 #32477 @hongchaodeng @xiang90 @lavalamp @smarterclayton
|
Commit found in the "release-1.4" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked. |
…ck-of-#31189-kubernetes#31704-kubernetes#32831-kubernetes#32907-kubernetes#33003-kubernetes#33349-kubernetes#31190-kubernetes#33581-kubernetes#34089-kubernetes#34234-kubernetes#32822-kubernetes#33393-kubernetes#34246-kubernetes#34435-kubernetes#32477-upstream-release-1.4 Automatic merge from submit-queue Automated cherry pick of kubernetes#31189 kubernetes#31704 kubernetes#32831 kubernetes#32907 kubernetes#33003 kubernetes#33349 kubernetes#31190 kubernetes#33581 kubernetes#34089 kubernetes#34234 kubernetes#32822 kubernetes#33393 kubernetes#34246 kubernetes#34435 kubernetes#32477 upstream release 1.4 We are going to release etcd v3 in 1.4.x release. ``` Cherrypick etcd v3-related bug fixes to 1.4 branch ``` Those PRs include: - Updates of etcd Godeps - Update to pkg/storage/etcd directory - Two PR that were unnecessary to avoid conflicts: kubernetes#31189 kubernetes#31704 Automated cherry pick of kubernetes#31189 kubernetes#31704 kubernetes#32831 kubernetes#32907 kubernetes#33003 kubernetes#33349 kubernetes#31190 kubernetes#33581 kubernetes#34089 kubernetes#34234 kubernetes#32822 kubernetes#33393 kubernetes#34246 kubernetes#34435 kubernetes#32477 @hongchaodeng @xiang90 @lavalamp @smarterclayton
Ref #33653
With the "Cacher" layer in Kubernetes, most of the watches processed by "pkg/storage/etcd3/watcher.go" have "filter = Everything()". That said, we generally don't need to decode previous value of the object (which is used only to get the value of filter of it), because we already know it will be true.
This PR is basically fixing this problem.
Should be merged after #34246
This change is