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

[python] Avoid creating unused ThreadPools #8061

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

minrk
Copy link

@minrk minrk commented Apr 23, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Instead, create ApiClient.pool on first request for .pool property.

This avoids spawning n-cpus threads (the default for ThreadPool) at instantiation of every ApiClient, which can be very costly in, for example, container scenarios on nodes with many CPUs where multiple API Clients might be instantiated in one process. kubernetes unconditionally instantiates an ApiClient only for serialization that never makes any requests every time a Watch is created, for example.

The thread count is made configurable and defaults to 1 instead of N-CPUs, which I think makes more sense for the default behavior of a non-blocking API.

@minrk
Copy link
Author

minrk commented Apr 27, 2018

cc @taxpon @frol @mbohlool @cbornet @kenjones-cisco for Python review

@frol
Copy link
Contributor

frol commented Apr 27, 2018

LGTM, but it doesn't seem like you did:

Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows.

@minrk
Copy link
Author

minrk commented May 9, 2018

Sorry, failed to commit it before. Looks like this had been missed a few times, since there are a few more changes in the results than mine.

@minrk
Copy link
Author

minrk commented May 9, 2018

Had to fix a deserialization test that hadn't been updated since some changes in enums, I think. The failing test passed locally for me, hopefully Travis will find the same.

@kenjones-cisco
Copy link
Contributor

LGTM

@minrk
Copy link
Author

minrk commented Jun 13, 2018

Test failure appears to have been an intermittent network issue. Anything I can to do help this progress?

@minrk
Copy link
Author

minrk commented Aug 21, 2018

Resolved conflicts.

@007
Copy link

007 commented Nov 5, 2018

This blocks on exit the majority of times when more than one API client is created (kubernetes-client/python#411). What needs to be done to get this merged and released so the k8s Python API can be regenerated and fixed?

007 added a commit to 007/nedry that referenced this pull request Nov 5, 2018
Instead, create ApiClient.pool on first request for .pool property.

avoids spawning n-cpus threads (the default for ThreadPool) at instantiation of every ApiClient
and default to 1 for putting requests in the background
without triggering concurrency.
by running bin/python-*.sh
@minrk
Copy link
Author

minrk commented Nov 6, 2018

Rebased and rerendered again

@007
Copy link

007 commented Apr 27, 2019

Happy anniversary #8061 🎉

mmourafiq added a commit to polyaxon/polyaxon that referenced this pull request Apr 15, 2020
…enapi-generator.

 * related
      * kubernetes-client/gen#93
      * swagger-api/swagger-codegen#8061
      * OpenAPITools/openapi-generator#1387
 * Drop hack for empty reponses

 Bump versions:
  * sdks to 1.0.79
  * core to 1.0.74
@noamgat
Copy link

noamgat commented Nov 18, 2020

Was this ever merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants