-
Notifications
You must be signed in to change notification settings - Fork 36
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
[4.x] Schema queries with configurable server-side timeouts #312
[4.x] Schema queries with configurable server-side timeouts #312
Conversation
core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/api/core/config/DefaultDriverOption.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/api/core/metadata/Node.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultTopologyMonitor.java
Outdated
Show resolved
Hide resolved
I don't fully understand the whole Scylla version issue here. 5.0/2022.1 are ancient version. If someone is using such an old version, what happens when we try to fetch the schema with timeout? |
I couldn't figure out a clean way to get Scylla version without querying I probably should've marked this as draft for now since I'm still cleaning this up. I may switch to either checking for shard awareness (as a proxy check for "is it scylla cluster") or running trial query with |
What do you mean too old? What happen when you do try to use timeout, and the scylla version is too old? If the timeout is ignored - fine, go with it. Doesn't matter. If it crashes, I'm more worried. If it errs the CQL query - then I think you should fallback to not using timeout.
Why? What would happen when you use it against a Cassandra cluster? Same as the above, IMHO (just as we try initially with high DSE versions that are not supported against Scylla!) |
Cassandra does not have |
I'd catch the exception and re-run the query without 'USING TIMEOUT' . Probably a more optimistic approach (happy path) then the whole additional query we have. Of course, we may use the Scylla version or whatnot elsewhere, later, etc, so it might have value by itself. |
4221556
to
492557b
Compare
I've switched to running small trial select with Commit with querying for Scylla version remains as it was. While it is no longer necessary for this change I also believe it may be useful later and its impact should be minimal. Waiting to see CI results. |
492557b
to
c984c31
Compare
core/src/main/java/com/datastax/oss/driver/api/core/metadata/Node.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/internal/core/metadata/DefaultTopologyMonitor.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/internal/core/metadata/NodeInfo.java
Outdated
Show resolved
Hide resolved
core/src/main/java/com/datastax/oss/driver/api/core/metadata/Node.java
Outdated
Show resolved
Hide resolved
...src/test/java/com/datastax/oss/driver/internal/core/metadata/DefaultTopologyMonitorTest.java
Outdated
Show resolved
Hide resolved
c984c31
to
3d13226
Compare
I've removed commit with |
I don't understand what's happening with gh actions. Unit tests fail due to cancellation but I have no idea what is triggering it. |
Oh, it's just timeout. I guess it became too small at some point. |
...va/com/datastax/oss/driver/internal/core/metadata/schema/queries/CassandraSchemaQueries.java
Show resolved
Hide resolved
3d13226
to
be26134
Compare
Unit tests should pass now |
1316cce
to
f5234ef
Compare
I couldn't understand why my test query was causing integration tests to take too long, so instead I look at sharding info of the control connection channel. If it's non-null I am assuming I'm dealing with Scylla and will use the timeouts. I don't check the Scylla version. |
This looks much better - please resolve all comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit message says USING TIMESTAMP
instead of USING TIMEOUT
Apart from that looks good to me
The driver will now use existing configurable option `METADATA_SCHEMA_REQUEST_TIMEOUT` in a `USING TIMEOUT` clause with each schema query (if applicable). This will make it a server side timeout in addition to driver side. The driver will apply the `USING` clause if the sharding info of control connection channel is not null. This is used as a proxy check to differentiate between Scylla and Cassandra. The behaviour should remain unchanged for Cassandra clusters. USING TIMEOUT is a ScyllaDB CQL extension and it is available in versions 4.4/2022.1.0 or newer.
f5234ef
to
bfd340e
Compare
I've fixed the typo @Lorak-mmk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. One more thing: shouldn't we do the same with topology queries?
Is topology response large? I doubt it, but we can certainly test it with a (mockup) large setup. |
There are as many rows as nodes in the cluster. What is the biggest cluster that we know of? |
Adds USING TIMEOUT clause to schema queries when applicable.
Uses the same timeout as driver-side timeouts defined by
METADATA_SCHEMA_REQUEST_TIMEOUT
("advanced.metadata.schema.request-timeout").Fixes #298