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

Improve connection towards Azure #121

Merged
merged 3 commits into from Jan 16, 2024
Merged

Conversation

ace-e4s
Copy link
Member

@ace-e4s ace-e4s commented Jan 14, 2024

This PR is related to user story ESS-XXXX

Description

Establish and reuse requests.Session object in connection towards Azure. In this was, the underlying TCP connection is reused and potentially result in more performant and stable network connection. (See, https://docs.python-requests.org/en/latest/user/advanced/#session-objects)

The effect is marginal and barely observable in practice (normal office conditions), since there are other bottlenecks in the process. Nonetheless, an improvement of the code quality wrt network connection performance.

Checklist

  • PR title is descriptive and fit for injection into release notes (see tips below)
  • Correct label(s) are used

PR title tips:

  • Use imperative mood
  • Describe the motivation for change, issue that has been solved or what has been improved - not how
  • Examples:
    • Add functionality for Allan variance to sensor_4s.simulate
    • Upgrade to support Python 9.10
    • Remove MacOS from CI

@bjorn-einar-bjartnes-4ss

The one thing we need to make sure of when reusing connections is that they are re-created if it fails when IP changes, over longer periods (reusing connections in long-running processes) the connection must be recreated if DNS is changing in the blob store. I assume this is handled by retries on the methods itself, but since the non-idempotent methods such as PUT and POST to blobs I have to read up on the details to be sure.

@@ -15,6 +15,14 @@

log = logging.getLogger(__name__)

_AZURE_SESSION = requests.Session()

Choose a reason for hiding this comment

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

Is this really a blobstorage or blobstorageaccount session? All of our resources are in azure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean we should call it BLOBSTORAGE_SESSION to be more precise? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Just keep the underscore prefix, and call it whatever you like :)

Copy link
Contributor

Choose a reason for hiding this comment

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

renamed to BLOBSTORAGE_SESSION now

@branislav-jenco-4ss
Copy link
Contributor

Regarding retries over DNS failures, from what I read it should be handled, though I haven't tested.

@ace-e4s
Copy link
Member Author

ace-e4s commented Jan 16, 2024

All low level network stuff is handled by urllib3. Almost certain that fundamentals are covered.

@ace-e4s
Copy link
Member Author

ace-e4s commented Jan 16, 2024

Theoretically, we run at the risk of leaving a session (and all the related low level resources) open and allocated. But, since its one session per Python kernel, I'm sure it wont have an impact overall.

@branislav-jenco-4ss
Copy link
Contributor

LGTM

@branislav-jenco-4ss branislav-jenco-4ss merged commit 4261440 into master Jan 16, 2024
9 checks passed
@branislav-jenco-4ss branislav-jenco-4ss deleted the azure-session-improve branch January 16, 2024 15:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

4 participants