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

Schema queries with configurable server-side timeouts #222

Merged
merged 2 commits into from
Jul 31, 2024

Conversation

sylwiaszunejko
Copy link
Collaborator

@sylwiaszunejko sylwiaszunejko commented Jul 24, 2024

Adds USING TIMEOUT clause to schema queries when applicable.
Uses the timeout defined by MetadataSchemaRequestTimeout option in ClusterConfig.

Fixes: #172
Fixes: #157

@sylwiaszunejko
Copy link
Collaborator Author

I am not sure how to check if USING TIMEOUT should be applied. Every query to schema tables is executed through control connection, so it made same to me to put usingTimeoutClause in controlConn. An easy way to check if scylla is used is to use isScyllaConn https://github.com/scylladb/gocql/blob/master/scylla.go#L285, but it takes Conn not controlConn. I am not sure how to solve this.

Copy link
Collaborator

@dkropachev dkropachev left a comment

Choose a reason for hiding this comment

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

Looks great.

Copy link
Collaborator

@dkropachev dkropachev left a comment

Choose a reason for hiding this comment

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

Let's cover queryes from awaitSchemaAgreement, querySystemPeers and querySystemLocal

@sylwiaszunejko
Copy link
Collaborator Author

@dkropachev do you have any suggestions regarding differentiating between scylla and cassandra from the position of Session or controlConn? scylladb/java-driver#312 (comment)

@dkropachev
Copy link
Collaborator

Idea on testing it:

  1. Set timeout to something that will break queries, 1ns
  2. Establish session and make sure that schema is there and routing works properly.
  3. Double check that you regular queries are failing by timeout

@dkropachev
Copy link
Collaborator

dkropachev commented Jul 24, 2024

@dkropachev do you have any suggestions regarding differentiating between scylla and cassandra from the position of Session or controlConn? scylladb/java-driver#312 (comment)

  1. Have a flag to disable this feature in ClusterConfig
  2. Have separate flag to disable it on session level.
  3. Test it out on Session.Init() and set the flag to disable feature if syntax error is received.

Perfect place for it is:

gocql/session.go

Lines 236 to 238 in c26ee43

s.control.getConn().conn.mu.Lock()
s.tabletsRoutingV1 = s.control.getConn().conn.isTabletSupported()
s.control.getConn().conn.mu.Unlock()

@sylwiaszunejko
Copy link
Collaborator Author

Let's cover queryes from awaitSchemaAgreement, querySystemPeers and querySystemLocal

I was following scylladb/java-driver#312 and there such queries were not covered. @Bouncheck am I right?

@dkropachev
Copy link
Collaborator

Let's cover queryes from awaitSchemaAgreement, querySystemPeers and querySystemLocal

I was following scylladb/java-driver#312 and there such queries were not covered. @Bouncheck am I right?

It was not.

Why I think it makes sense to cover them too:
Bigger idea of this feature is to keep driver sane when cluster is under high preassure and/or when certain queries are taking a long time.
So, any query that is ran by the driver for it's internal needs to be covered.
Also, we don't loose much doing that, at worst driver will slow down instead of failing.

@mykaul
Copy link

mykaul commented Jul 24, 2024

Let's cover queryes from awaitSchemaAgreement, querySystemPeers and querySystemLocal

I was following scylladb/java-driver#312 and there such queries were not covered. @Bouncheck am I right?

It was not.

Why I think it makes sense to cover them too: Bigger idea of this feature is to keep driver sane when cluster is under high preassure and/or when certain queries are taking a long time. So, any query that is ran by the driver for it's internal needs to be covered. Also, we don't loose much doing that, at worst driver will slow down instead of failing.

Those are not large queries, IMHO:

private static final String SELECT_SCHEMA_PEERS =
      "SELECT peer, rpc_address, schema_version, host_id FROM system.peers";
  private static final String SELECT_SCHEMA_LOCAL =
      "SELECT schema_version, host_id FROM system.local WHERE key='local'";

@dkropachev
Copy link
Collaborator

Let's cover queryes from awaitSchemaAgreement, querySystemPeers and querySystemLocal

I was following scylladb/java-driver#312 and there such queries were not covered. @Bouncheck am I right?

It was not.
Why I think it makes sense to cover them too: Bigger idea of this feature is to keep driver sane when cluster is under high preassure and/or when certain queries are taking a long time. So, any query that is ran by the driver for it's internal needs to be covered. Also, we don't loose much doing that, at worst driver will slow down instead of failing.

Those are not large queries, IMHO:

private static final String SELECT_SCHEMA_PEERS =
      "SELECT peer, rpc_address, schema_version, host_id FROM system.peers";
  private static final String SELECT_SCHEMA_LOCAL =
      "SELECT schema_version, host_id FROM system.local WHERE key='local'";

True, but since code is already there it won't take much effort to cover them too.

@sylwiaszunejko sylwiaszunejko marked this pull request as ready for review July 25, 2024 09:20
@sylwiaszunejko
Copy link
Collaborator Author

Let's cover queryes from awaitSchemaAgreement, querySystemPeers and querySystemLocal

Done, and some simple tests added

@sylwiaszunejko
Copy link
Collaborator Author

@dkropachev could you take a look?

schema_queries_test.go Outdated Show resolved Hide resolved
roydahan
roydahan previously approved these changes Jul 29, 2024
Copy link
Collaborator

@roydahan roydahan left a comment

Choose a reason for hiding this comment

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

Overall LGTM, but I would wait to @dkropachev and @Lorak-mmk to have their review.

session.go Outdated Show resolved Hide resolved
schema_queries_test.go Outdated Show resolved Hide resolved
dkropachev
dkropachev previously approved these changes Jul 29, 2024
@Lorak-mmk Lorak-mmk self-assigned this Jul 30, 2024
@Lorak-mmk Lorak-mmk removed their assignment Jul 31, 2024
roydahan
roydahan previously approved these changes Jul 31, 2024
@sylwiaszunejko sylwiaszunejko merged commit c33cb52 into scylladb:master Jul 31, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants