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

[MISC] Add experimental max_connections config #472

Merged
merged 3 commits into from
May 21, 2024
Merged

Conversation

dragomirp
Copy link
Contributor

@dragomirp dragomirp commented May 20, 2024

Adds experimental support for force setting max_connections.

@@ -1468,7 +1468,7 @@ def _restart(self, event: RunWithLock) -> None:
return

try:
for attempt in Retrying(wait=wait_fixed(3), stop_after_delay=stop_after_delay(300)):
for attempt in Retrying(wait=wait_fixed(3), stop=stop_after_delay(300)):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typo™

Comment on lines +1550 to +1555
# Use config value if set, calculate otherwise
if self.config.experimental_max_connections:
max_connections = self.config.experimental_max_connections
else:
max_connections = max(4 * os.cpu_count(), 100)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing this only here would trigger a restart on deploy with experimental_max_connections. I can also inject it in the config file, by changing the internal library, but I would rather keep it KISS.

@dragomirp dragomirp marked this pull request as ready for review May 20, 2024 19:55
Comment on lines +405 to +408
experimental_max_connections:
type: int
description: |
[EXPERIMENTAL] Force set max_connections.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we add some kind of validation here?

Copy link
Member

Choose a reason for hiding this comment

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

should we maybe enforce an upper limit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a footgun config.

Copy link
Member

@lucasgameiroborges lucasgameiroborges left a comment

Choose a reason for hiding this comment

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

LGTM!

@dragomirp dragomirp merged commit 4feeaee into main May 21, 2024
53 checks passed
@dragomirp dragomirp deleted the max-connections branch May 21, 2024 12:52
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.

3 participants