search

kubernetes-sigs / kubebuilder-declarative-pattern

A toolkit for building declarative operators with kubebuilder
Apache License 2.0
256 stars 85 forks source link

Optimization: Reuse HTTPClient / DynamicClient in reconcile #371

Closed justinsb closed 8 months ago

justinsb commented 8 months ago

Should be slightly more efficient via better HTTP connection reuse.

k8s-ci-robot commented 8 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/kubebuilder-declarative-pattern/blob/master/OWNERS)~~ [justinsb] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
justinsb commented 8 months ago

cc @cheftako - as requested re reusing the DynamicClient; I think the answer is that the caller should try to provide a cached DynamicClient, because client-go is more efficient if we can reuse the http.Client. Added a comment and ensured that k-d-p itself passes the dynamic client (which we actually already have!)

atoato88 commented 8 months ago

First of all, this looks good.

BTW, I am not sure about my comment is related with this PR. :thinking: Current direct applier uses the dynamicClient at here. This always uses the dynamicClient from Factory. Should we update the direct.go to use the opt.DynamicClient?

On the other hand, dynamicClient in applylib.go takes care of the existance of opt.DynamicClient. It's OK as-is, I think.

justinsb commented 8 months ago

Great point @atoato88, and you're absolutely right. I added a commit that should also reuse the dynamic-client in the direct applier.

(I don't think we can practically reuse it in the exec applier.)

I also noticed that we probably should be trying to reuse the http client here, but I think I'll have to send a separate PR for that one!

atoato88 commented 8 months ago

@justinsb Thank you for the response and update.

(I don't think we can practically reuse it in the exec applier.)

I think so too.

I also noticed that we probably should be trying to reuse the http client here, but I think I'll have to send a separate PR for that one!

OK! We can go forward. :rocket:

atoato88 commented 8 months ago

/lgtm