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

fix: first try/guess fixing race of requests #51

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

skoegl
Copy link
Member

@skoegl skoegl commented Apr 26, 2024

This PR is a draft - not tested yet...

  • configure the session to use more connections allowed in connection pool determined by core count
  • change simple assert check to full try except with better insight whats going on on low level errors on heavy load

@skoegl skoegl added discussion These topics must be discussed before completion proposal labels Apr 26, 2024
@skoegl skoegl marked this pull request as draft April 26, 2024 11:16
@sveneberth
Copy link
Member

I tested it with the following config:

from viur.datastore.transport import _http_internal
from requests.adapters import HTTPAdapter, DEFAULT_POOLSIZE
from multiprocessing import cpu_count

_http_internal.mount(
    "https://",
    HTTPAdapter(
        pool_connections=max(DEFAULT_POOLSIZE, cpu_count() * 4 * 2),
        pool_maxsize=max(DEFAULT_POOLSIZE, cpu_count() * 4 * cpu_count()),
        max_retries=3,
    )
)

In the cloud with the F2 instance this is cpu_count=2 and therefore pool_connections=16 and pool_maxsize=16.

Google has the following settings: https://github.com/googleapis/google-auth-library-python/blob/8cfc91db0861bc92374d708140656fb38e003ef6/google/auth/transport/requests.py#L409-L410
And requests itself has the default poolsize of 10: https://github.com/requests/requests-docs-de/blob/6e58fc50174cf463866081835da253fb37c318e4/requests/adapters.py#L65

We should therefore not go below these values in any case. But I haven't noticed any noticeable difference. So you don't have to use it as my configuration.

But I find the part with the assert the most interesting anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion These topics must be discussed before completion proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants