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

share kubernetes client instances #128

Merged
merged 3 commits into from
Feb 23, 2018
Merged

Conversation

minrk
Copy link
Member

@minrk minrk commented Jan 30, 2018

avoids creating a kubernetes client for each Spawner, which creates a bunch of threads in kubernetes 4.0.

uses weakref to avoid breaking garbage collection

closes #126

@minrk minrk changed the title share kubernetes client instances [WIP] share kubernetes client instances Jan 30, 2018
@yuvipanda
Copy link
Collaborator

we should hold off on this until after 0.6 of z2jh.

@minrk
Copy link
Member Author

minrk commented Jan 30, 2018

yes, I was planning to cut the release and finish z2jh before merging this

avoids creating a kubernetes client for each Spawner,
which creates a bunch of threads in kubernetes 4.0.

uses weakref to avoid breaking garbage collection
@yuvipanda
Copy link
Collaborator

@minrk is this good to go or still WIP?

cache_key = (ClientType, args, kwarg_key)
client = None
if cache_key in _client_cache:
client = _client_cache[cache_key]()
Copy link
Member

Choose a reason for hiding this comment

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

Why the trailing ()? I think _client_cache contains instances already so this would end up calling __call__ on that instance.

Copy link
Member Author

Choose a reason for hiding this comment

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

_client_cache contains weak references to instances, not the instances themselves. This allows them to be garbage collected. The ref must be called in order to return the instance. If it has been garbage collected in the meantime, resolving the weakref will return None.

I've added a comment to clarify what's happening here.

I've

@minrk minrk changed the title [WIP] share kubernetes client instances share kubernetes client instances Feb 23, 2018
@minrk
Copy link
Member Author

minrk commented Feb 23, 2018

Forgot to take off WIP that was meant to block it until shipping v0.6. This should be all set now.

@betatim
Copy link
Member

betatim commented Feb 23, 2018

Merging as travis is happy.

Thanks for explaining the weakref thing.

@betatim betatim merged commit f7ebb59 into jupyterhub:master Feb 23, 2018
@minrk minrk deleted the share-apiclient branch February 26, 2018 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Share kubernetes client objects
3 participants