Add information about server application version to metadata #1307
Conversation
This is the server version used by the Scylla/C* node.
This is a non-breaking change, because `Peer` is non_exhaustive.
This is a non-breaking change, because `Node` already has some private fields. I decided not to include `server_version` in `PeerEndpoint` to avoid needless cloning. To obtain the server_version from `Peer`, one can use `Peer::into_peer_endpoint_tokens_and_server_version()`. This consumes the `Peer` and moves the `server_version` to the caller.
In cpp-driver, `cass_schema_meta_version` returns the output of `release_version` column from `system.local` query executed on current control connection. I'm not entirely sure if we should expose such API. `ClusterState::get_nodes_info()` would be enough to implement in on the cpp-driver side - we would just choose the version of random/first node. Or maybe return an error if there is a version mismatch (?). This can be discussed. The main thing I hate about this commit is this ugly `fold` expression in `query_peers_and_cluster_version`. I have no idea if there is any cleaner solution to this.
|
|
muzarski
force-pushed
the
server-release-version
branch
from
d436fa7 to
7fd38dc
Compare
muzarski
requested review from
Lorak-mmk and
wprzytula
and removed request for
Lorak-mmk
There was a problem hiding this comment.
I did not review the code carefully yet, this is just a general comment.
I think that having the version in ClusterState too (so not only in Node) makes sense, because it makes the field much better to use. One change I'd do in this regard is putting a comment saying that the semantic of this field may change in the future, for example we may:
- Use
Noneif not all nodes have the same version - Concat versions if not all are the same to represent cluster that is being upgraded.
This PR makes sense for Cassandra, and for cpp-rust-driver, but is not very useful for Scylla users, who are our primary target.
Maybe we could expose Scylla version in ClusterState too?
I see that it is available for local node in system.versions (table existing only on Scylla).
We would need to make sure that this table is stable enough to be used in the driver (cc @piodul ).
| /// This test will work only for Scylla, as currently `release_version` | ||
| /// column always contains '3.0.8' value. | ||
| /// See: https://github.com/scylladb/scylladb/issues/8740 | ||
| #[cfg_attr(cassandra_tests, ignore)] | ||
| #[tokio::test] | ||
| async fn test_metadata_scylla_release_version() { | ||
| setup_tracing(); | ||
|
|
||
| let session = create_new_session_builder().build().await.unwrap(); | ||
| let hardcoded_scylla_version = "3.0.8"; | ||
|
|
||
| // Get release_version manually. | ||
| let release_version = session | ||
| .query_unpaged( | ||
| "SELECT release_version FROM system.local WHERE key='local'", | ||
| &[], | ||
| ) | ||
| .await | ||
| .unwrap() | ||
| .into_rows_result() | ||
| .unwrap() | ||
| .single_row::<(String,)>() | ||
| .unwrap() | ||
| .0; | ||
| assert_eq!(&release_version, hardcoded_scylla_version); | ||
|
|
||
| let cluster_state = session.get_cluster_state(); | ||
|
|
||
| let cluster_version = cluster_state.cluster_version().unwrap(); | ||
| assert_eq!(cluster_version, hardcoded_scylla_version); | ||
|
|
||
| for node in cluster_state.get_nodes_info() { | ||
| assert_eq!( | ||
| node.server_version.as_deref(), | ||
| Some(hardcoded_scylla_version) | ||
| ); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
You can make this test work with C* too by getting rid of hardcoded version and using the result of manuala select as the source of truth.
Fixes: #1286
This PR adds server application version information to the metadata.
It is exposed in two places:
Nodestruct - a server version of specific node (release_versionfromsystem.local/system.peers)ClusterState- a cluster version, which is the version of the node currently used for control connection (release_versionfromsystem.local)I'm not entirely sure about the second place. Maybe it's not really needed. I added some context to the corresponding commit message, so we can discuss this.
I implemented the test which validates the version for Scylla. It's possible, because Scylla currently hardcodes the
release_versioncolumn to3.0.8and it does not seem like it's going to be addressed in the near future (the reason is some java-driver compatibility issues).Pre-review checklist
[ ] I have adjusted the documentation in./docs/source/.Fixes:annotations to PR description.