Skip to content

Fix metadata request timeout #539

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 29, 2025
Merged

Fix metadata request timeout #539

merged 3 commits into from
Sep 29, 2025

Conversation

dkropachev
Copy link
Collaborator

@dkropachev dkropachev commented Sep 26, 2025 •
edited
Loading

When I was testing e4a000f, I have figure that out there is another option that controls system queries timeout - Cluster.control_connection_timeout, switch to use that it instead of Cluster.metadata_request_timeout, but did not remove Cluster.metadata_request_timeout.

Effectively Cluster.metadata_request_timeout is not working, what is working is Cluster.control_connection_timeout.
This PR addresses it the following way:

  1. Makes Cluster.metadata_request_timeout actually work
  2. Defaults Cluster.metadata_request_timeout from Cluster.control_connection_timeout

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

@dkropachev dkropachev mentioned this pull request Sep 26, 2025
Closed
8 tasks
Lorak-mmk
Lorak-mmk reviewed Sep 26, 2025
View reviewed changes
Copy link

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

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

control_connection_timeout is 2s by default. Should we maybe increase it?

@dkropachev
Copy link
Collaborator Author

control_connection_timeout is 2s by default. Should we maybe increase it?

100%, let's address it separately, read - https://github.com/scylladb/scylla-drivers/issues/106

@Lorak-mmk
Copy link

Actually wait - maybe default for server-side should not be exactly the same as client side, but slightly bigger? If it's the same, then server-side will never be shown to useelr due to network latency (client side will trigger first).

@dkropachev
Copy link
Collaborator Author

Actually wait - maybe default for server-side should not be exactly the same as client side, but slightly bigger? If it's the same, then server-side will never be shown to useelr due to network latency (client side will trigger first).

Yes, it should be slightly bigger to make up for network and processing delays.
IMHO this difference is negligible (say 20ms vs 2s) when timeout is high enough, which is general case, I can't think of any case that can require lower it down to ms range.

@dkropachev
cluster: default metadata_request_timeout to control_connection_timeout
48c7f40 
metadata_request_timeout and control_connection_timeout are going hand
by hand, control_connection_timeout is client side timeout, while
metadata_request_timeout is server side.
In general case they always have to be eual.
@dkropachev
control_connection: read actual metadata_request_timeout from Cluster…
24de1d5 
….metadata_request_timeout

PR scylladb#361 that have introduced `metadata_request_timeout` intentionally
pulled actual `metadata_request_timeout` from
`control_connection_timeout`, because both `metadata_request_timeout`
and `control_connection_timeout` has to be inline.
Since this PR brings `Cluster.metadata_request_timeout` back, we need to pull it
from `Cluster.metadata_request_timeout`
@dkropachev
tests: add more integration tests for metadata_request_timeout
c6e127a 
New behavior needs to be tested:
1. metadata_request_timeout can be borrowed from
   control_connection_timeout
2. metadata_request_timeout can be set to 0, to disable it.
3. metadata_request_timeout can be set to some value to override
   default.
4. When both metadata_request_timeout and control_connection_timeou it
   should be disabled
@dkropachev dkropachev force-pushed the dk/fix_metadata_request_timeout branch from f147330 to c6e127a Compare September 26, 2025 16:23
@dkropachev
Copy link
Collaborator Author

@Lorak-mmk , can you please re-review again and reaprove if everything is fine.

@dkropachev dkropachev merged commit 51dbe8b into scylladb:master Sep 29, 2025
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lorak-mmk Lorak-mmk Lorak-mmk approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dkropachev @Lorak-mmk