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

Remove MissingUserDefinedType from public API #1162

Merged
merged 6 commits into from
Jan 22, 2025

Conversation

Lorak-mmk
Copy link
Collaborator

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.

@Lorak-mmk Lorak-mmk self-assigned this Dec 29, 2024
@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Dec 29, 2024
Copy link

github-actions bot commented Dec 29, 2024

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

See the following report for details:

cargo semver-checks output
./scripts/semver-checks.sh --baseline-rev a216932412c375fc81a5737257edfe2236ac3e08
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev a216932412c375fc81a5737257edfe2236ac3e08
     Cloning a216932412c375fc81a5737257edfe2236ac3e08
    Building scylla v0.15.0 (current)
       Built [  22.639s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.052s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  22.443s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.049s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.108s] 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::cluster::metadata::MissingUserDefinedType, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-a216932412c375fc81a5737257edfe2236ac3e08/e302ccb9dfc77c2b9b626ab3e35888a4548dda73/scylla/src/cluster/metadata.rs:289

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  46.389s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  10.894s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.034s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  11.035s] (baseline)
     Parsing scylla-cql v0.4.0 (baseline)
      Parsed [   0.034s] (baseline)
    Checking scylla-cql v0.4.0 -> v0.4.0 (no change)
     Checked [   0.110s] 107 checks: 107 pass, 0 skip
     Summary no semver update required
    Finished [  22.668s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

Copy link
Contributor

@muzarski muzarski left a comment

Choose a reason for hiding this comment

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

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.

scylla/src/transport/topology.rs Outdated Show resolved Hide resolved
scylla/src/transport/topology.rs Outdated Show resolved Hide resolved
scylla/src/transport/topology.rs Outdated Show resolved Hide resolved
scylla/src/transport/topology.rs Outdated Show resolved Hide resolved
scylla/src/transport/cluster.rs Outdated Show resolved Hide resolved
@Lorak-mmk Lorak-mmk force-pushed the remove-missing-udt-error branch from d8ecc8d to 71379da Compare January 4, 2025 17:26
@Lorak-mmk Lorak-mmk force-pushed the remove-missing-udt-error branch from 71379da to 3ec27b9 Compare January 5, 2025 11:08
wprzytula
wprzytula previously approved these changes Jan 8, 2025
@Lorak-mmk
Copy link
Collaborator Author

Rebased on #1163

@Lorak-mmk Lorak-mmk requested a review from wprzytula January 8, 2025 16:32
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` 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.
@Lorak-mmk Lorak-mmk force-pushed the remove-missing-udt-error branch from 5d1eb8b to 800aabc Compare January 20, 2025 19:47
@Lorak-mmk
Copy link
Collaborator Author

Rebased on main - should be ready to merge after getting an approve.

Copy link
Contributor

@muzarski muzarski left a comment

Choose a reason for hiding this comment

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

Looks good!

@Lorak-mmk
Copy link
Collaborator Author

@wprzytula ping

@Lorak-mmk Lorak-mmk merged commit 4a9367c into scylladb:main Jan 22, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants