Remove MissingUserDefinedType from public API #1162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
semver-checks-breaking
|
See the following report for details: cargo semver-checks output |
muzarski
reviewed
Dec 30, 2024
There was a problem hiding this comment.
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.
wprzytula
requested changes
Dec 31, 2024
Lorak-mmk
force-pushed
the
remove-missing-udt-error
branch
from
d8ecc8d to
71379da
Compare
As Connection's methods are mainly intended for private use by Connection itself or by the abstractions from connection_pool.rs exclusively, their visibility was reduced to `pub(self)` or `pub(super)`, respectively. Let's make our assumptions about layer separation enforced by the module visibility rules.
perform_authenticate takes `&mut Connection` as its first argument, which makes it a perfect suit to become a method on Connection.
As other drivers do, Rust driver will hold all user-implementable policies in one module for easier access by interested code-reading users.
Lorak-mmk
force-pushed
the
remove-missing-udt-error
branch
from
71379da to
3ec27b9
Compare
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.
wprzytula
previously approved these changes
Jan 8, 2025
Lorak-mmk
force-pushed
the
remove-missing-udt-error
branch
from
3ec27b9 to
5d1eb8b
Compare
|
Rebased on #1163 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Up until now
CqlType::UserDefinedTypecontained aResult, with error typeMissingUserDefinedType.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:
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_schemawas unnecessarily called twice, so I deduplicated those calls.query_tables_schemais 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 added relevant tests for new features and bug fixes.I have provided docstrings for the public items that I want to introduce.I have adjusted the documentation in./docs/source/.Fixes:annotations to PR description.