connection: expose setters for self-identity STARTUP options #1039

wprzytula · 2024-07-10T15:55:50Z

Resolves: #1028

Motivation

See #1028.

What's done

Preparatory steps

Reduce allocations by using `Cow<str>` for SUPPORTED/STARTUP options

types::write_string_map was genericised to accept HashMaps with keys and values being any form of strings, including Cow<str>.
as majority of keys and significant number of values of STARTUP options are known at compile time and thus statically allocated, Startup request now holds HashMap<Cow<str>, Cow<str> instead of HashMap<String, String> to reduce needless allocations.

refactors for readability

setup_tcp_keepalive() was extracted from Connection::new() to reduce clutter.
keys of SUPPORTED/STARTUP options were extracted as constants to options module to have them in one place and avoid using plain string literals in code.
resolved a dated TODO about whether logic in open_named_connection should be part of Connection::new - answered the question, justified the answer and added docstrings to convey that answer.
added many comments to open_named_connection, as the function is big and crucial for driver operation, so future readers will benefit.

Compression-related small enhancements

Compression name is now no longer allocated, as its name is a &'static str anyway.
warning is now emitted if requested Compression is not supported; before, it would silently fall back to no compression.

`SelfIdentity` struct and API

grouped self-identity-related STARTUP options in SelfIdentity struct. It includes:
- DRIVER_NAME
- DRIVER_VERSION
- APPLICATION_NAME
- APPLICATION_VERSION
- CLIENT_ID
DRIVER_NAME and DRIVER_VERSION are set by default to "ScyllaDB Rust Driver" (previously "scylla-rust-driver", changed to be consistent with "ScyllaDB Python Driver") and Rust driver's package version as declared by Cargo. The remaining ones are unset by default.
exposed SelfIdentity's API in SessionBuilder, and added a docstring with an example usage.

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.

github-actions · 2024-07-10T16:00:36Z

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: 9aadac8

muzarski

Good job on adding comments to connection module. I always feel like I'm lost trying to understand the logic that resides there.

The commit message mentions:

Settable identity include:
- DRIVER_NAME
- DRIVER_VERSION
- APPLICATION_NAME
- APPLICATION_VERSION
- CLIENT_ID

All five are visible in Cassandra's `system_views.clients` in
`client_options` column.

DRIVER_NAME & DRIVER_VERSION are visible in ScyllaDB's `system.clients`.

Maybe we should add some test cases that verify it? Especially for the newly introduced STARTUP options.

Lorak-mmk · 2024-07-12T19:34:28Z

Where does APPLICATION_NAME and APPLICATION_VERSION come from?
PR description reads like those attributes are as supported and known as DRIVER_NAME / DRIVER_VERSION, but:

there are no columns in system.clients dedicated to those attributes
APPLICATION_NAME / APPLICATION_VERSION are not present in Scylla's source code at all
only 2 results in Cassandra source is some example in docs: https://github.com/apache/cassandra/blob/d3cbf9c1f72057d2a5da9df8ed567d20cd272931/doc/modules/cassandra/pages/managing/operating/virtualtables.adoc?plain=1#L218 . APPLICATION_NAME and APPLICATION_VERSION appears in client_options which IIUC is an arbitrary dict where client can send any keys.
driver variables are mentioned in protocol v5 (https://github.com/apache/cassandra/blob/d3cbf9c1f72057d2a5da9df8ed567d20cd272931/doc/native_protocol_v5.spec#L480 ), application variables are not.

Please explain in the PR description (and maybe commit message too):

Why support those specific keys instead of arbitrary keys
The difference in support between driver variables and application variables
Where those application variables come from.

scylla-cql/src/frame/request/options.rs

Lorak-mmk · 2024-07-12T20:22:47Z

scylla/src/transport/connection.rs

+            tracing::warn!(
+                "Requested compression <{}> is not supported by the cluster. Falling back to no compression",
+                compression_str
+            );


when can this happen?

This may happen if SUPPORTED message sent by a node does not contain the particular compression string in the value for the "COMPRESSION" key.

scylla/src/transport/session_builder.rs

scylla/src/transport/connection.rs

This enables serializing maps with non-necessarily owned kinds of string, e.g. HashMap<&str, &str> or HashMap<Cow<str>, Cow<str>>.

Majority of, if not all, keys sent in the string map in STARTUP message by the driver are known in the compile time. Some values may not be known at compile time, but still it makes more sense to borrow them from a String rather than clone each time a connection is opened. To reduce needless allocations while preserving ability to add keys and values dynamically, the HashMap type is changed to `HashMap<Cow<'a, str>, Cow<'a, str>>`. This is going to reduce number of allocations upon opening each connection.

The tcp keepalive logic cluttered Connection::new(). I extracted it to a separate function.

wprzytula · 2024-08-01T10:03:16Z

Rebased on main.

As these strings bear strict meaning specified by the CQL protocol, it makes sense to have them in one place as constants.

Before, `scylla-rust-driver` name was used. First, we should move to to ScyllaDB because of the established naming. Second, Python driver advertises itself as "ScyllaDB Python Driver", so let's be consistent.

Answering the question there: no, that logic shouldn't be in `Connection::new`. `Connection::new`'s responsibility is to create a connection and make it ready to send CQL frames. OTOH, `open_named_connection` is responsible for connection setup on CQL level, i.e. for OPTIONS/STARTUP handshake and registering for events (if this is a control connection). Informative docstrings are added that convey the above separation.

Compression's name is known in compile time, so let's not allocate it needlessly.

Before, the driver would silently fall back to no compression.

The function is quite big and at the same time crucial for driver's operation, so comments there will bring more ease to future readers.

wprzytula · 2024-08-01T10:37:38Z

Where does APPLICATION_NAME and APPLICATION_VERSION come from? PR description reads like those attributes are as supported and known as DRIVER_NAME / DRIVER_VERSION, but:

* there are no columns in system.clients dedicated to those attributes

* `APPLICATION_NAME` / `APPLICATION_VERSION` are not present in Scylla's source code at all

* only 2 results in Cassandra source is some example in docs: https://github.com/apache/cassandra/blob/d3cbf9c1f72057d2a5da9df8ed567d20cd272931/doc/modules/cassandra/pages/managing/operating/virtualtables.adoc?plain=1#L218 . `APPLICATION_NAME` and `APPLICATION_VERSION` appears in `client_options` which IIUC is an arbitrary dict where client can send any keys.

* driver variables are mentioned in protocol v5 (https://github.com/apache/cassandra/blob/d3cbf9c1f72057d2a5da9df8ed567d20cd272931/doc/native_protocol_v5.spec#L480 ), application variables are not.

Please explain in the PR description (and maybe commit message too):

* Why support those specific keys instead of arbitrary keys

* The difference in support between driver variables and application variables

* Where those application variables come from.

They come directly from CPP driver. See https://github.com/scylladb/cpp-driver/blob/fa0f27069a625057984d1fa58f434ea99b86c83f/include/cassandra.h#L2916.
As we want to support as big subset of its API as possible in cpp-rust-driver, I decided to expose API for setting those particular key-value pairs, similarly to what cpp-driver does, and not an API to set arbitrary key-value pairs.

Allowing users to set arbitrary options could break the driver by overwriting options that bear special meaning, e.g. the shard-aware port. Therefore, I'm against such liberal API. OTOH, we need to expose APPLICATION_NAME, APPLICATION_VERSION and CLIENT_ID for cpp-rust-driver.

Does this convince you @Lorak-mmk?

Lorak-mmk · 2024-08-01T10:39:36Z

Where does APPLICATION_NAME and APPLICATION_VERSION come from? PR description reads like those attributes are as supported and known as DRIVER_NAME / DRIVER_VERSION, but:
* there are no columns in system.clients dedicated to those attributes

* `APPLICATION_NAME` / `APPLICATION_VERSION` are not present in Scylla's source code at all

* only 2 results in Cassandra source is some example in docs: https://github.com/apache/cassandra/blob/d3cbf9c1f72057d2a5da9df8ed567d20cd272931/doc/modules/cassandra/pages/managing/operating/virtualtables.adoc?plain=1#L218 . `APPLICATION_NAME` and `APPLICATION_VERSION` appears in `client_options` which IIUC is an arbitrary dict where client can send any keys.

* driver variables are mentioned in protocol v5 (https://github.com/apache/cassandra/blob/d3cbf9c1f72057d2a5da9df8ed567d20cd272931/doc/native_protocol_v5.spec#L480 ), application variables are not.
Please explain in the PR description (and maybe commit message too):
* Why support those specific keys instead of arbitrary keys

* The difference in support between driver variables and application variables

* Where those application variables come from.
They come directly from CPP driver. See https://github.com/scylladb/cpp-driver/blob/fa0f27069a625057984d1fa58f434ea99b86c83f/include/cassandra.h#L2916. As we want to support as big subset of its API as possible in cpp-rust-driver, I decided to expose API for setting those particular key-value pairs, similarly to what cpp-driver does, and not an API to set arbitrary key-value pairs.

Allowing users to set arbitrary options could break the driver by overwriting options that bear special meaning, e.g. the shard-aware port. Therefore, I'm against such liberal API. OTOH, we need to expose APPLICATION_NAME, APPLICATION_VERSION and CLIENT_ID for cpp-rust-driver.

Does this convince you @Lorak-mmk?

It does. Please include this explanation in commit message and in a comment.

SelfIdentity is intended to group STARTUP options that can be used to uniquely identify driver, application and particular client instance. DRIVER_NAME and DRIVER_VERSION were already present, but they were hardcoded to "scylla-rust-driver" and current crate version, retrieved from Cargo. Now it is possible to set custom values for them, which can be useful for custom driver builds, as well as for drivers running on top of Rust driver (think cpp-rust-driver). The remaining options are inspired by cpp-driver, and added for cpp-rust-driver purposes as well: - APPLICATION_NAME and APPLICATION_VERSION identify a single build of an application using the driver. - CLIENT_ID uniquely identifies a single instance of an application. All the above information are visible in Cassandra in `system_views.clients` in `client_options` column. DRIVER_NAME and DRIVER_VERSION are visible in ScyllaDB in `system.clients`. FAQ Q: Where do APPLICATION_NAME, APPLICATION_VERSION and CLIENT_ID come from? - there are no columns in system.clients dedicated to those attributes, - APPLICATION_NAME / APPLICATION_VERSION are not present in Scylla's source code at all, - only 2 results in Cassandra source is some example in docs; APPLICATION_NAME and APPLICATION_VERSION appears in client_options which is an arbitrary dict where client can send any keys. - driver variables are mentioned in protocol v5, application variables are not. A: The following options are not exposed anywhere in Scylla tables. They come directly from CPP driver, and they are supported in Cassandra. As we want to support as big subset of its API as possible in cpp-rust-driver, I decided to expose API for setting those particular key-value pairs, similarly to what cpp-driver does, and not an API to set arbitrary key-value pairs. Allowing users to set arbitrary options could break the driver by overwriting options that bear special meaning, e.g. the shard-aware port. Therefore, I'm against such liberal API. OTOH, we need to expose APPLICATION_NAME, APPLICATION_VERSION and CLIENT_ID for cpp-rust-driver.

The exposed option enabled setting custom identity that is advertised in STARTUP message by the driver, upon opening each connection. Settable identity include: - DRIVER_NAME - DRIVER_VERSION - APPLICATION_NAME - APPLICATION_VERSION - CLIENT_ID All five are visible in Cassandra's `system_views.clients` in `client_options` column. DRIVER_NAME & DRIVER_VERSION are visible in ScyllaDB's `system.clients`.

wprzytula · 2024-08-01T13:42:06Z

v2:

addressed review suggestions;
made SelfIdentity more convenient by accepting impl Into<Cow<...>> instead of only Cow<...>;
wrote a test using proxy that asserts proper self identity being set in STARTUP frame.

wprzytula requested review from Lorak-mmk and muzarski and removed request for Lorak-mmk July 10, 2024 15:55

wprzytula added enhancement New feature or request cpp-rust-driver-p0 Functionality required by cpp-rust-driver labels Jul 10, 2024

wprzytula self-assigned this Jul 10, 2024

Lorak-mmk self-assigned this Jul 10, 2024

wprzytula force-pushed the self-identity-customisation branch from 6ab7774 to bffad0d Compare July 10, 2024 16:10

muzarski suggested changes Jul 10, 2024

View reviewed changes

Lorak-mmk requested changes Jul 12, 2024

View reviewed changes

Lorak-mmk removed their assignment Jul 31, 2024

wprzytula added 3 commits August 1, 2024 12:03

frame/types: genericise write_string_map

dad5559

This enables serializing maps with non-necessarily owned kinds of string, e.g. HashMap<&str, &str> or HashMap<Cow<str>, Cow<str>>.

connection: extract setup_tcp_keepalive()

a5d4af8

The tcp keepalive logic cluttered Connection::new(). I extracted it to a separate function.

wprzytula force-pushed the self-identity-customisation branch from bffad0d to a8ab3f3 Compare August 1, 2024 10:03

wprzytula added 7 commits August 1, 2024 12:25

connection: extract OPTIONS keys as constants

15de00f

As these strings bear strict meaning specified by the CQL protocol, it makes sense to have them in one place as constants.

connection: extract default OPTIONS values as constants

49a712c

connection: advertise as "ScyllaDB Rust Driver"

aed83f4

Before, `scylla-rust-driver` name was used. First, we should move to to ScyllaDB because of the established naming. Second, Python driver advertises itself as "ScyllaDB Python Driver", so let's be consistent.

connection: don't allocate Compression name

c90f3a9

Compression's name is known in compile time, so let's not allocate it needlessly.

connection: warn if requested Compression is not supported

850b449

Before, the driver would silently fall back to no compression.

connection: add comments to open_named_connection

8f5eec1

The function is quite big and at the same time crucial for driver's operation, so comments there will bring more ease to future readers.

wprzytula force-pushed the self-identity-customisation branch from a8ab3f3 to 0ea5a4e Compare August 1, 2024 11:44

wprzytula added 2 commits August 1, 2024 15:39

tests: integration test for custom self identity

9aadac8

wprzytula force-pushed the self-identity-customisation branch from 0ea5a4e to 9aadac8 Compare August 1, 2024 13:39

wprzytula requested review from Lorak-mmk and muzarski August 1, 2024 13:43

muzarski approved these changes Aug 1, 2024

View reviewed changes

Lorak-mmk approved these changes Aug 2, 2024

View reviewed changes

wprzytula merged commit 36d5edd into scylladb:main Aug 3, 2024
11 checks passed

wprzytula deleted the self-identity-customisation branch August 3, 2024 07:32

wprzytula changed the title ~~connection: expose setters for STARTUP options~~ connection: expose setters for self-identity STARTUP options Aug 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

connection: expose setters for self-identity STARTUP options #1039

connection: expose setters for self-identity STARTUP options #1039

wprzytula commented Jul 10, 2024

github-actions bot commented Jul 10, 2024 •

edited

Loading

muzarski left a comment

Lorak-mmk commented Jul 12, 2024

Lorak-mmk Jul 12, 2024

wprzytula Aug 1, 2024

wprzytula commented Aug 1, 2024

wprzytula commented Aug 1, 2024

Lorak-mmk commented Aug 1, 2024

wprzytula commented Aug 1, 2024

connection: expose setters for self-identity STARTUP options #1039

connection: expose setters for self-identity STARTUP options #1039

Conversation

wprzytula commented Jul 10, 2024

Motivation

What's done

Preparatory steps

Reduce allocations by using Cow<str> for SUPPORTED/STARTUP options

refactors for readability

Compression-related small enhancements

SelfIdentity struct and API

Pre-review checklist

github-actions bot commented Jul 10, 2024 • edited Loading

muzarski left a comment

Choose a reason for hiding this comment

Lorak-mmk commented Jul 12, 2024

Lorak-mmk Jul 12, 2024

Choose a reason for hiding this comment

wprzytula Aug 1, 2024

Choose a reason for hiding this comment

wprzytula commented Aug 1, 2024

wprzytula commented Aug 1, 2024

Lorak-mmk commented Aug 1, 2024

wprzytula commented Aug 1, 2024

Reduce allocations by using `Cow<str>` for SUPPORTED/STARTUP options

`SelfIdentity` struct and API

github-actions bot commented Jul 10, 2024 •

edited

Loading