errors: final cleanups (restructure ExecutionError and introduce SchemaAgreementError) #1204
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
force-pushed
the
inline-request-error
branch
2 times, most recently
from
8a7a9e7 to
f23611c
Compare
Lorak-mmk
reviewed
Feb 7, 2025
scylla/src/errors.rs
Outdated
Comment on lines
3
to
36
| // Re-export error types from pager module. | ||
| pub use crate::client::pager::{NextPageError, NextRowError}; | ||
|
|
||
| // Re-export error types from query_result module. | ||
| pub use crate::response::query_result::{ | ||
| FirstRowError, IntoRowsResultError, MaybeFirstRowError, ResultNotRowsError, RowsError, | ||
| SingleRowError, | ||
| }; | ||
|
|
||
| // Re-export error type from authentication module. | ||
| pub use crate::authentication::AuthError; | ||
|
|
||
| // Re-export error types from scylla-cql. | ||
| pub use scylla_cql::deserialize::{DeserializationError, TypeCheckError}; | ||
| pub use scylla_cql::frame::frame_errors::{ | ||
| CqlAuthChallengeParseError, CqlAuthSuccessParseError, CqlAuthenticateParseError, | ||
| CqlErrorParseError, CqlEventParseError, CqlRequestSerializationError, CqlResponseParseError, | ||
| CqlResultParseError, CqlSupportedParseError, FrameBodyExtensionsParseError, | ||
| FrameHeaderParseError, | ||
| }; | ||
| pub use scylla_cql::frame::request::CqlRequestKind; | ||
| pub use scylla_cql::frame::response::error::{DbError, OperationType, WriteType}; | ||
| pub use scylla_cql::frame::response::CqlResponseKind; | ||
| pub use scylla_cql::serialize::SerializationError; |
There was a problem hiding this comment.
🤔 Re-exporting errors from scylla_cql of course makes sense. I'm wondering about errors from scylla. Should we re-export them here, or maybe move them here?
And if we decide to use re-exports, which errors should be defined in the errors module, and which whould only be re-exported here?
There was a problem hiding this comment.
🔧 In any case, please move re-exports after imports.
There was a problem hiding this comment.
🔧 In any case, please move re-exports after imports.
Done
There was a problem hiding this comment.
🤔 Re-exporting errors from scylla_cql of course makes sense. I'm wondering about errors from
scylla. Should we re-export them here, or maybe move them here?
I'm fine with either approach.
And if we decide to use re-exports, which errors should be defined in the errors module, and which whould only be re-exported here?
I believe errors such as NextRowError/IntoRowsResultError were introduced in different module, since they are strictly tied to one object (in these cases it's QueryPager and QueryResult). Errors in errors module are rather related to methods on Session. But I'm pretty sure this does not apply to all errors in this module (e.g. RequestError used only in RetrySession API). So, actually, I think we could move ALL error types to scylla::errors module. Current division is a bit misleading.
This commit inlines the RequestAttemptError in ExecutionError. It also adjusts some tests which otherwise are failing. This is because some variants land in other place than they would before this commit. In the next commits we will start cleaning up the ExecutionError - i.e. removing unused variants and moving some of them to other places. The purpose why we inline RequestAttemptError is to remove some error variants from the top level. For example, `CqlErrorParseError` should not be placed on the top level of the error hierarchy. It is a very specific and users should get some more context about it - i.e. that this is the cause of failure of the last attempt.
I removed the variants that can be removed "for free", i.e. no further changes in the codebase are required.
Removed the variant and adjusted the tests.
This commit moves the NonfinishedPagingState variant from the ProtocolError to RequestAttemptError (apologize for the short names in the title, otherwise it wouldn't fit in one line in GitHub ui). Why we do this? The conversion (e.g. NonErrorQueryResponse::into_query_result) methods for response kinds are currently a bit problematic. Some of them return `RequestAttemptError` (e.g. QueryResponse::into_non_error_query_response), while some of them are forced to return `ExecutionError` (e.g. [NonError]QueryResponse::into_query_result) because of the aforementioned variant, which was placed in ProtocolError. Why is this problematic? Well, these conversion methods are called from different contexts: - in Session layer, at the end of execution path - it makes sense to return ExecutionError there, because it's the final error type returned from the execution API. - in Connection layer, in pub(crate) methods (e.g. Connection::query_unpaged) that are used internally by e.g. schema agreement logic. It's not a good idea to extend the error type to ExecutionError in such case. Connection methods should return errors representing an error of a single attempt (i.e. RequestAttemptError). Our goal is to narrow the return error type of `Connection::fetch_schema_version` to RequestAttemptError. This method calls `Connection::query_unpaged`, which then calls `QueryResponse::into_query_result`. In result, `Connection::fetch_schema_version` is forced to return ExecutionError. Starting from the following commit, we will narrow the return type of methods related to schema agreement. We will introduce a self-contained error type, i.e. SchemaAgreementError, which can be returned from public API (Session::await_schema_agreement).
The idea behind it was explained in the previous commit.
Few things happen here. 1. Introducing new error type SchemaAgreementTimeout This is rather a rename of `SchemaVersionFetch` error type. We extend the type by additional variants. 2. Removing ProtocolError::SchemaVersionFetch variant 3. Narrowing the error type of schema agreement methods Thanks to that, public API methods (e.g. Session::await_schema_agreement) can return a self-contained error type providing more context than previously returned ExecutionError. 4. Replacing ExecutionError::SchemaAgreementTimeout with SchemaAgreementError variant SchemaAgreementError dependency is still needed, because of auto await schema agreement configuration. By default, the driver awaits schema agreement on schema altering queries. 5. Adjusting the example Previously, the example would (unjustifiably) match against ExecutionError::RequestTimeout error. Even before this PR, it should match against ExecutionError::SchemaAgreementTimeout. Thanks to this commit, we were able to spot and fix this issue.
I believe that this is currently the best fitting place for this variant.
We have removed the ProtocolError type, which was previously used as a catch-all for various sub-errors and variants. It was something like "I don't know where it belongs, so I'm gonna put it under ProtocolError".
So the order resembles the order of variants construction during execution.
So they are accessible via scylla::errors
muzarski
force-pushed
the
inline-request-error
branch
from
f23611c to
6b271fb
Compare
Lorak-mmk
reviewed
Feb 7, 2025
There was a problem hiding this comment.
I noticed some pub types without non_exhaustive. Please tell me if they should remain that way, have non_exhaustive added, or be unpubed.
FirstRowErrorIntoRowsResultErrorMaybeFirstRowErrorResultNotRowsErrorRowsErrorSingleRowError
cc @wprzytula because they come from deserialization refactor.
Also:
DbError(!)OperationTypeWriteType
We also have
/// Type to represent an authentication error message.
pub type AuthError = String;is it indentional?
I remember asking about it during deserialization PRs. I don't remember why we decided not to mark them as
These should definitely be
Why wouldn't it be? We could always convert it to |
I'm not saying it is wrong, I'm just asking if it is intentionally this way or was just overlooked. |
Ok. So would you leave it as a pub AuthError(Arc<dyn Error>);
// Errors returned by driver's default implementations of the trait (if there are any - I'm not sure)
pub enum BuiltinAuthErrorKind {
...
}This would look like the |
|
Let's leave it as string. |
I extracted the part of speculative_execution::can_be_ignored as DbError method. This way, we don't have to match against "_" (because method is defined in the same module as the type - namely scylla_cql).
|
v1.1:
We still need to discuss and agree on whether we want to pack all the error types used in the driver to |
Lorak-mmk
approved these changes
Feb 10, 2025
wprzytula
approved these changes
Feb 10, 2025
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.
Fixes: #519
This PR originated from our offline discussion with @Lorak-mmk .
Changes
ProtocolError
We remove this type altogether. We noticed that this error type was a catch-all for sub-errors/variants we did not know where to put otherwise.
ExecutionError
Before this PR, it would contain a lot of top-level variants without providing some meaningful context for them. Most of the variants came from the
RequestAttemptError::into_execution_errormethod.Inlining RequestAttemptError
We decided that it's best to inline
RequestAttemptError, so users have additional context about how request execution looks like when they match the error. It allows us to remove a lot of variants from ExecutionError and ProtocolError (which is later removed).SchemaAgreementError
New error type - returned by public API related to awaiting schema agreement (Session::await/check_schema_agreement).
BadQuery
Me and @Lorak-mmk agreed that final version (after this PR) of
BadQueryis just fine. I did not do any changes to this error type, apart from moving one of the ProtocolError variants to it. I remember that @wprzytula did not like the name of this error - any suggestions?Potential improvements for the future
SerializationError appears in two places
BadQuery::SerializationErrorThis is an error that appears on standard execution path for
Session::executeandSession::batch, when we serialize the values (before doing the retries etc.)ExecutionError::LastAttemptError(RequestAttemptError::SerializationError)This error is constructed ONLY for
Session::querywith non-empty values. Why do we currently need a SerializationError in RequestAttemptError - i.e. why do we serialize on each attempt? This is because we need to firstly prepare the statement during each attempt - the statement needs to be firstly prepared on the node during current attempt. Only after preparation, have we access to the metadata and are able to serialize the values.After discussion, we decided to currently leave it as is. This is something to be potentially fixed in the future. The fix requires non-trivial changes to execution logic (which is quite complex). The idea is to prepare the statement only once before the retries - then we can serialize the values and pass the serialized values to the context of each attempt. I think it deserves a separate issue in the repository, since it not only will fix the hierarchy of errors, but will improve the overall performance as well - we won't need to prepare the statement on each attempt (we can send QUERY cql request to the node).
Pre-review checklist
[ ] I added relevant tests for new features and bug fixes.[ ] I have adjusted the documentation in./docs/source/.Fixes:annotations to PR description.