Decouple schema fetch queries timeouts from server side timeouts #361

dkropachev · 2024-08-08T20:00:19Z

Scylla have an ability to override server timeout by appending USING TIMEOUT <timeout>ms to the query
Make driver add USING TIMEOUT <control_connection_timeout*1000>ms for scylla connections

Closes #320

tests/unit/test_util_types.py

tests/unit/advanced/test_metadata.py

fruch

Test classes need to have Test in thier name

Lorak-mmk · 2024-08-20T14:51:32Z

cassandra/cluster.py

+                 shard_aware_options=None,
+                 metadata_request_timeout=datetime.timedelta(seconds=2),
+                 ):


I think it will be less confusing if the default is in one place (definition of the field) and here it is just None)

Lorak-mmk · 2024-08-20T14:58:23Z

cassandra/metadata.py

+class SchemaQueryMessage(QueryMessage):
+    def __init__(self, query, consistency_level, serial_consistency_level=None,
+                 fetch_size=None, paging_state=None, timestamp=None, continuous_paging_options=None, keyspace=None,
+                 custom_timeout=None):
+        super(SchemaQueryMessage, self).__init__(
+            add_timeout_to_query(query, custom_timeout), consistency_level,
+            serial_consistency_level=serial_consistency_level, fetch_size=fetch_size, paging_state=paging_state,
+            timestamp=timestamp, continuous_paging_options=continuous_paging_options, keyspace=keyspace)
+
+


Why super(SchemaQueryMessage, self).__init__ instead of super().__init__? I tried to research it quickly, and internet says that in Python 3 those are equal.

Parameter name should be something like server_side_timeout - right now it is not clear which timeout is it for someone reading the code.

A comment explaining what this class is would be appreciated. It's not obvious to me why you are introducing new class instead of adding parameter to QueryMessage contructor.

Also consider introducing it in a separate commit. I'd like to see this PR split into commits because it is not easy to digest as one commit. Possible way to split:

Introduce SchemaQueryMessage, explaining its purpose, and change usages of QueryMessage to SchemaQueryMessage (without using custom_timeout parameter for now)

Add timeout parameter to I think Metadata.refresh - or whatever is the place from which it is taken to be passed to SchemaQueryMessage

Add the plumbing in Cluster - this is the moment that the new feature is actually used.

Add tests

I'll wait with further review until the PR is split into commits to be more digestable.

@Lorak-mmk , I have dropped SchemaQueryMessage, and split PR into commits, please take a look

roydahan · 2024-09-16T11:46:56Z

@dkropachev what's the status of this PR?

dkropachev · 2024-09-16T11:55:04Z

@dkropachev what's the status of this PR?

I have postponed working on this PR due to the higher priority tasks on my side and @Lorak-mmk being focused on rust driver.
This week I will address comments.

roydahan · 2024-09-23T11:09:50Z

@dkropachev let's try to merge this by the end of quarter if possible.

dkropachev · 2024-09-30T11:18:37Z

aarch64 tests failing on test_multi_timer_validation, this test is known to be flaky on this architecture.

Lorak-mmk · 2024-10-01T12:29:35Z

cassandra/metadata.py

+    def __init__(self, connection, timeout, fetch_size, metadata_request_timeout=None):
+        super(SchemaParserV22, self).__init__(connection, timeout, fetch_size, metadata_request_timeout)
        self.keyspaces_result = []
        self.tables_result = []


SchemaParser* structs are not part of public interface I think, se we should be able to add new mandatory argument to __init__. Wouldn't it be better for metadata_request_timeout to be such an argument, like timeout is? There is less room for mistakes on the caller side with mandatory arguments, and you are always passing the argument anyway.

Lorak-mmk · 2024-10-01T12:40:13Z

cassandra/util.py

+
+def add_timeout_to_query(stmt: str, metadata_request_timeout: datetime.timedelta) -> str:
+    if metadata_request_timeout is None:
+        return stmt
+    ms = int(metadata_request_timeout / datetime.timedelta(milliseconds=1))
+    if ms == 0 or _query_timeout_regexp.search(stmt) is not None:
+        return stmt
+    return f"{stmt} USING TIMEOUT {ms}ms"


if metadata_request_timeout can be None then the type should be Optional[datetime.timedelta].

I really wish we could sit down and add types to the whole driver and then run pyright (or other type checker) in CI.

Lorak-mmk · 2024-10-01T12:41:06Z

cassandra/util.py

+
+def add_timeout_to_query(stmt: str, metadata_request_timeout: datetime.timedelta) -> str:
+    if metadata_request_timeout is None:
+        return stmt
+    ms = int(metadata_request_timeout / datetime.timedelta(milliseconds=1))


Better name for this function would be maybe_add_timeout_to_query as it doesn't always add the timeout.

Lorak-mmk · 2024-10-01T12:58:44Z

cassandra/util.py

+
+
+_query_timeout_regexp = re.compile('USING[ \t]+TIMEOUT[ \t]+[0-9]+[a-z]+[ \t]*;?$')
+


We control all the places where the add_timeout_to_query function is called, there should be no need to have such a regex and performs checks with it.

If this is supposed to be a more universal function (and thus requiring this regex) then there are some problems with it (some of them unsolvable):

For INSERT there may be no literal USING TIMEOUT. INSERT INTO ks.t2 (pk, ck) VALUES (1, 2) USING TIMESTAMP 12345678 AND TIMEOUT 360ms; is a valid statement. I don't think it's fixable using regular expressions.

As soon as some new element of SELECT is added (and USING TIMEOUT doesn't have to be at the end of the statement) similar problem as above happens for SELECTs.

Time unit can use uppercase letters, so [a-z] is wrong.

USING TIMEOUT is case insensitive.

agree, at the worst we will see that query fails on the integration and unit tests, regexp removed.

This option allows user to control timeout for driver internal queries. Idea is to make driver queries more resilient and being independent of user queries.

dkropachev linked an issue Aug 8, 2024 that may be closed by this pull request

Python implementation of "Decouple schema fetch queries from server-side timeouts" #320

Open

dkropachev force-pushed the dk/320-python-implementation-of-decouple-schema-fetch-queries-from-server-side-timeouts branch 5 times, most recently from d94edcc to 31d490a Compare August 9, 2024 00:06

dkropachev marked this pull request as ready for review August 9, 2024 01:03

dkropachev requested review from sylwiaszunejko and Lorak-mmk August 9, 2024 01:03

dkropachev self-assigned this Aug 9, 2024

fruch reviewed Aug 11, 2024

View reviewed changes

tests/unit/test_util_types.py Outdated Show resolved Hide resolved

fruch reviewed Aug 11, 2024

View reviewed changes

tests/unit/advanced/test_metadata.py Show resolved Hide resolved

dkropachev force-pushed the dk/320-python-implementation-of-decouple-schema-fetch-queries-from-server-side-timeouts branch from 31d490a to ea6ed21 Compare August 11, 2024 07:29

dkropachev requested a review from fruch August 16, 2024 02:15

fruch reviewed Aug 16, 2024

View reviewed changes

dkropachev force-pushed the dk/320-python-implementation-of-decouple-schema-fetch-queries-from-server-side-timeouts branch from ea6ed21 to d1f38d8 Compare August 16, 2024 12:33

dkropachev requested a review from fruch August 16, 2024 12:34

Lorak-mmk requested changes Aug 20, 2024

View reviewed changes

dkropachev force-pushed the dk/320-python-implementation-of-decouple-schema-fetch-queries-from-server-side-timeouts branch 5 times, most recently from d40a956 to cbfc3fb Compare September 29, 2024 22:26

dkropachev requested a review from Lorak-mmk September 30, 2024 11:17

Lorak-mmk requested changes Oct 1, 2024

View reviewed changes

Introduce metadata_request_timeout configuration option

e4a000f

This option allows user to control timeout for driver internal queries. Idea is to make driver queries more resilient and being independent of user queries.

Use metadata_request_timeout for all driver queries

4beebd5

dkropachev force-pushed the dk/320-python-implementation-of-decouple-schema-fetch-queries-from-server-side-timeouts branch 2 times, most recently from ed356aa to 5b0a63f Compare October 3, 2024 11:25

Test metadata_request_timeout configuration option

7a4ae44

dkropachev force-pushed the dk/320-python-implementation-of-decouple-schema-fetch-queries-from-server-side-timeouts branch from 5b0a63f to 7a4ae44 Compare October 3, 2024 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple schema fetch queries timeouts from server side timeouts #361

Decouple schema fetch queries timeouts from server side timeouts #361

dkropachev commented Aug 8, 2024 •

edited

Loading

fruch left a comment

Lorak-mmk Aug 20, 2024

dkropachev Sep 27, 2024

Lorak-mmk Aug 20, 2024

dkropachev Sep 27, 2024 •

edited

Loading

roydahan commented Sep 16, 2024

dkropachev commented Sep 16, 2024

roydahan commented Sep 23, 2024

dkropachev commented Sep 30, 2024

Lorak-mmk Oct 1, 2024

dkropachev Oct 3, 2024

Lorak-mmk Oct 1, 2024

dkropachev Oct 3, 2024

Lorak-mmk Oct 1, 2024

dkropachev Oct 3, 2024

Lorak-mmk Oct 1, 2024

dkropachev Oct 3, 2024 •

edited

Loading



		_query_timeout_regexp = re.compile('USING[ \t]+TIMEOUT[ \t]+[0-9]+[a-z]+[ \t]*;?$')

Decouple schema fetch queries timeouts from server side timeouts #361

Are you sure you want to change the base?

Decouple schema fetch queries timeouts from server side timeouts #361

Conversation

dkropachev commented Aug 8, 2024 • edited Loading

fruch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkropachev Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

roydahan commented Sep 16, 2024

dkropachev commented Sep 16, 2024

roydahan commented Sep 23, 2024

dkropachev commented Sep 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkropachev Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

dkropachev commented Aug 8, 2024 •

edited

Loading

dkropachev Sep 27, 2024 •

edited

Loading

dkropachev Oct 3, 2024 •

edited

Loading