Remove MissingUserDefinedType from public API #1162

Lorak-mmk · 2024-12-29T22:56:09Z

Up until now CqlType::UserDefinedType contained a Result, with error type MissingUserDefinedType.
This error is an edge case that should be quite rare. It could possibly happen if we fetch metadata in the middle of schema change.
Possible scenario:

We fetch UDTs
New UDT is added
New column is added to some table, with the type being this new UDT
We fetch column types for all tables
Now we have a column type with unknown UDT.

As this error should be extremely rare, it is okay to handle it a little bit less gracefully.
This simplifies user API (user no longer needs to handle the error) and makes progress towards unifying CqlType and ColumnType.

The new proposed way to handle this error is to drop the fetched metadata for the whole keyspace - re-using older version of it if available - and printing a warning.

As a side-fix, I found that query_tables_schema was unnecessarily called twice, so I deduplicated those calls.
query_tables_schema is likely the heaviest part of metadata reading, so this fix should lower the cluster load resulting from metadata fetches.

Progresses towards: #691

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.

Previously this method accepted a map with UDTs for all keyspaces. This is not necessary: UDTs in one keyspace can not reference UDTs in another keyspace.

This function performs a request that fetches columns for all tables and views. It is potentially the most performance-impactful part of the schema fetching process, and it was unnecessarily called twice: in query_tables and in query_views. It can be easily prevented by calling the function earlier and passing the result to query_tables and query_views.

MissingUserDefinedType was an obstacle preventing us from unifying ColumnType and CqlType. This commit changes the topology fetching code to remove the keyspace where such an error happened from the metadata. A warning is printed in such case.

It is no longer part of any public API.

This commit changes type of `keyspaces` field in `Metadata` to from `HashMap<String, Keyspace>` to `HashMap<String, Result<Keyspace, MissingUserDefinedType>>`. Because of that, it also removed `MissingUserDefinedType` handling from `query_metadata`. Now handling this error is done in `ClusterData::new`. This has an advantage: we can use older version of the keyspace metadata if the new version has this error.

github-actions · 2024-12-29T23:01:33Z

cargo semver-checks detected some API incompatibilities in this PR.
Checked commit: d8ecc8d

See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev 089f26f4913ed0f92e874a9b00f0b5bcdea69f86
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 089f26f4913ed0f92e874a9b00f0b5bcdea69f86
     Cloning 089f26f4913ed0f92e874a9b00f0b5bcdea69f86
    Building scylla v0.15.0 (current)
       Built [  23.788s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.054s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  21.916s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.053s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.123s] 107 checks: 106 pass, 1 fail, 0 warn, 0 skip

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.38.0/src/lints/struct_missing.ron

Failed in:
  struct scylla::transport::topology::MissingUserDefinedType, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-089f26f4913ed0f92e874a9b00f0b5bcdea69f86/5185b5c2bcd8e29dae8945e357a7480834050696/scylla/src/transport/topology.rs:272

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  46.985s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  10.929s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.035s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  10.787s] (baseline)
     Parsing scylla-cql v0.4.0 (baseline)
      Parsed [   0.037s] (baseline)
    Checking scylla-cql v0.4.0 -> v0.4.0 (no change)
     Checked [   0.161s] 107 checks: 107 pass, 0 skip
     Summary no semver update required
    Finished [  22.597s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

muzarski

Looks good. I really dislike how complicated all the methods have become. They accept hashmap where value is a Result, and return a really complicated type with nested Result. I don't see how we could simplify this, though. It looks horrible, but does the job - we can propagate the error for each keyspace up to ClusterData::new() thanks to this.

muzarski · 2024-12-30T10:11:43Z

scylla/src/transport/topology.rs

+        let mut tables_schema = query_tables_schema(conn, keyspaces_to_fetch, &udts).await?;
        (
-            query_tables(conn, keyspaces_to_fetch, &udts).await?,
-            query_views(conn, keyspaces_to_fetch, &udts).await?,
+            query_tables(conn, keyspaces_to_fetch, &mut tables_schema).await?,
+            query_views(conn, keyspaces_to_fetch, &mut tables_schema).await?,


Here we pass a mutable reference to tables_schema twice. Both query_tables and query_views remove items from the map. Are we guaranteed that we will try to remove each entry (keyspace, table/view) at most once? I think it makes sense to make such assumption (I don't think there can be a view with the same name as some table within one keyspace), but I'm asking just to be sure.

I launched a Scylla instance, created a keyspace, and inside this keyspace a table and a materialized view.
Materialized view was only present in system_schema.views (not in system_schema.tables) while the table was only present in system_schema.tables (not system_schema.views).
I can also check with a secondary index, and verify that it is the same for C*, but no other scenarios / tests come to my mind.

I think the scenario you checked is enough

muzarski · 2024-12-30T10:46:13Z

scylla/src/transport/topology.rs

 async fn query_views(
    conn: &Arc<Connection>,
    keyspaces_to_fetch: &[String],
-    tables: &mut HashMap<(String, String), Table>,
-) -> Result<HashMap<String, HashMap<String, MaterializedView>>, QueryError> {
+    tables: &mut HashMap<(String, String), Result<Table, MissingUserDefinedType>>,
+) -> Result<
+    HashMap<String, Result<HashMap<String, MaterializedView>, MissingUserDefinedType>>,
+    QueryError,
+> {


I'm really surprised that clippy does not complain about the complexity of return type. Do you think that introducing a documented generic type alias would aid readability of the code? The same pattern Result<HashMap<K, Result<V, MissingUserDefinedType>>, QueryError> appears in 3 different methods (query_[tables_schema/tables/views]).

I could introduce a type alias like type PerKeyspaceResult<T, E> = HashMap<String, Result<T, E>.
The return type here would become

Result<PerKeyspaceResult<HashMap<String, MaterializedView>, MissingUserDefinedType>, QueryError>

Other than that I am open to ideas about improving the simplicity of the code here.

wprzytula · 2024-12-30T11:01:11Z

scylla/src/transport/topology.rs

+        let (tables, views, user_defined_types) = match (tables, views, user_defined_types) {
+            (Ok(t), Ok(v), Ok(u)) => (t, v, u),
+            (Err(e), _, _) | (_, Err(e), _) | (_, _, Err(e)) => return Ok((keyspace_name, Err(e))),
+        };


🔧 Please add a comment justifying this logic.

wprzytula · 2024-12-31T10:34:13Z

scylla/src/transport/topology.rs

@@ -1507,9 +1566,42 @@ async fn query_tables_schema(
            return Ok::<_, QueryError>(());
        }

-        let keyspace_udts = udts.get(&keyspace_name).unwrap_or(&empty_map);
+        let keyspace_udts = match udts.get(&keyspace_name).unwrap_or(&empty_ok_map) {


🔧 I think a type annotation would aid readability here.

wprzytula · 2024-12-31T10:35:51Z

scylla/src/transport/cluster.rs

            &None,
            host_filter.as_deref(),
            TabletsInfo::new(),
+            &HashMap::new(),
        )


⛏️ typo in commit Move handling of MissingUserDefinedType to ClusterData::new: to from -> from

Lorak-mmk added 5 commits December 29, 2024 18:50

topology: accept keyspace udts in into_cql_type

fe9cf98

Previously this method accepted a map with UDTs for all keyspaces. This is not necessary: UDTs in one keyspace can not reference UDTs in another keyspace.

Topology: unpub MissingUserDefinedType

60a91b1

It is no longer part of any public API.

Lorak-mmk requested review from wprzytula and muzarski December 29, 2024 22:56

Lorak-mmk self-assigned this Dec 29, 2024

github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Dec 29, 2024

muzarski reviewed Dec 30, 2024

View reviewed changes

wprzytula requested changes Dec 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove MissingUserDefinedType from public API #1162

Remove MissingUserDefinedType from public API #1162

Lorak-mmk commented Dec 29, 2024

github-actions bot commented Dec 29, 2024

muzarski left a comment

muzarski Dec 30, 2024

Lorak-mmk Dec 30, 2024

muzarski Dec 31, 2024

muzarski Dec 30, 2024

Lorak-mmk Dec 30, 2024

wprzytula Dec 30, 2024

wprzytula Dec 31, 2024

wprzytula Dec 31, 2024

Remove MissingUserDefinedType from public API #1162

Are you sure you want to change the base?

Remove MissingUserDefinedType from public API #1162

Conversation

Lorak-mmk commented Dec 29, 2024

Pre-review checklist

github-actions bot commented Dec 29, 2024

muzarski 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

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