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 connection timeout #456

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Fix connection timeout #456

wants to merge 10 commits into from

Conversation

bram-vdberg
Copy link
Contributor

Currently the solver-rewards dry-run script returns fails in some environments due to the connection timing out. This PR adds some logging and explicitly sets the tcp_keepalives_idle value to 900 seconds.

@bram-vdberg bram-vdberg force-pushed the Fix-Connection-Timeout branch from e33059a to 83e2b0b Compare December 13, 2024 15:37
@bram-vdberg bram-vdberg marked this pull request as ready for review December 13, 2024 15:51
Copy link
Collaborator

@fhenneke fhenneke left a comment

Choose a reason for hiding this comment

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

I am not sure how to test there changes as they should not have an impackt on any local runs. Approving to not block testing this using other infrastructure.

@@ -58,10 +70,14 @@ def get_solver_rewards(

# Here, we use the convention that we run the prod query for the first connection
# and the barn query to all other connections
log.info("Setting tcp_keepalives_idle to 900 for prod connection")
self.connections[0].execute(text("SET tcp_keepalives_idle = 900;"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would probably introduce a constant for this value.

results.append(
self.exec_query(query=batch_reward_query_prod, engine=self.connections[0])
)
for engine in self.connections[1:]:
log.info("Running barn query on other connections (in get_solver_rewards")
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the long term plan here? It seems that not setting the time out for barn is fine. But it also makes it a bit more difficult to understand what is happening here.

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.

2 participants