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

implement Copy for Client? #17

Closed
clux opened this issue May 23, 2019 · 4 comments
Closed

implement Copy for Client? #17

clux opened this issue May 23, 2019 · 4 comments
Labels
client kube Client related question Direction unclear; possibly a bug, possibly could be improved.

Comments

@clux
Copy link
Member

clux commented May 23, 2019

we always clone this for passing between threads/informers anyway so might as well hide the clone call

@clux clux added the question Direction unclear; possibly a bug, possibly could be improved. label Jun 2, 2019
@clux clux changed the title implement Copy for Client implement Copy for Client? Jun 2, 2019
@clux
Copy link
Member Author

clux commented Mar 8, 2020

This isn't that big of an issue now that Informer's and Reflector's use Resource internally.

While reqwest::Client only clones the handles to the internal client, we do still clone a bunch of configuration. Perhaps it's best to not hide any magic like this.

@clux clux closed this as completed Mar 8, 2020
@clux
Copy link
Member Author

clux commented Nov 11, 2020

A quick grep through examples/ directory reveal 22 clones, 15 of which is the client, and the other mainly due to k8s-openapi being lax with Option. If our API requires that much cloning for just the simplest examples, then it will be quite distracting in real situations.

Saw tons of distracting clones today caused by our api in h2o mostly like this:

I think maybe we should rethink this. As it stands people are forced to think about whether to duplicate a Config, along with various Strings, Urls, for every Api needed, or for or parallel application.

Generally, I think, the small amount of memory this uses is not really a huge concern, it should be quickly reclaimed, and not likely to cause a bottleneck for a controller. BUT; it's an awkward thing to present in the api.

We could hide all this (everything's private anyway) in an Arc'd inner like reqwest does, and derive Copy

@clux clux reopened this Nov 11, 2020
@clux clux added the client kube Client related label Nov 11, 2020
@nightkr
Copy link
Member

nightkr commented Nov 11, 2020

You can't implement Copy if you have a custom Clone::clone (as Arc does).

@nightkr
Copy link
Member

nightkr commented Feb 7, 2021

Closing this as infeasible.

@nightkr nightkr closed this as completed Feb 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client kube Client related question Direction unclear; possibly a bug, possibly could be improved.
Projects
None yet
Development

No branches or pull requests

2 participants