From 85dab134f9c0d815eb74b5db7fb736112c2ec730 Mon Sep 17 00:00:00 2001 From: Bugen Zhao Date: Thu, 15 Feb 2024 11:43:09 +0800 Subject: [PATCH] refactor(connector): make `ConnectorError` a wrapper of `anyhow::Error` (#15086) Signed-off-by: Bugen Zhao --- Cargo.lock | 1 + src/connector/src/error.rs | 24 ++---- src/error/Cargo.toml | 1 + src/error/src/anyhow.rs | 146 +++++++++++++++++++++++++++++++++++++ src/error/src/lib.rs | 1 + 5 files changed, 155 insertions(+), 18 deletions(-) create mode 100644 src/error/src/anyhow.rs diff --git a/Cargo.lock b/Cargo.lock index 8f91a6aa97020..7c89a6da1f53f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9145,6 +9145,7 @@ dependencies = [ name = "risingwave_error" version = "1.7.0-alpha" dependencies = [ + "anyhow", "bincode 1.3.3", "bytes", "easy-ext", diff --git a/src/connector/src/error.rs b/src/connector/src/error.rs index db773add56364..4cf36e9859d36 100644 --- a/src/connector/src/error.rs +++ b/src/connector/src/error.rs @@ -12,26 +12,14 @@ // See the License for the specific language governing permissions and // limitations under the License. -use thiserror::Error; +use risingwave_common::error::v2::def_anyhow_newtype; -#[derive(Error, Debug)] -pub enum ConnectorError { - #[error("MySQL error: {0}")] - MySql( - #[from] - #[backtrace] - mysql_async::Error, - ), +def_anyhow_newtype! { + pub ConnectorError, - #[error("Postgres error: {0}")] - Postgres(#[from] tokio_postgres::Error), - - #[error(transparent)] - Uncategorized( - #[from] - #[backtrace] - anyhow::Error, - ), + // TODO(error-handling): Remove implicit contexts below and specify ad-hoc context for each conversion. + mysql_async::Error => "MySQL error", + tokio_postgres::Error => "Postgres error", } pub type ConnectorResult = Result; diff --git a/src/error/Cargo.toml b/src/error/Cargo.toml index 13bb50a371853..4a99711db6c41 100644 --- a/src/error/Cargo.toml +++ b/src/error/Cargo.toml @@ -8,6 +8,7 @@ license = { workspace = true } repository = { workspace = true } [dependencies] +anyhow = "1" bincode = "1" bytes = "1" easy-ext = "1" diff --git a/src/error/src/anyhow.rs b/src/error/src/anyhow.rs new file mode 100644 index 0000000000000..0acadddd88fa3 --- /dev/null +++ b/src/error/src/anyhow.rs @@ -0,0 +1,146 @@ +// Copyright 2024 RisingWave Labs +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +/// Define a newtype wrapper around [`anyhow::Error`]. +/// +/// # Usage +/// +/// ```ignore +/// def_anyhow_newtype! { +/// /// Documentation for the newtype. +/// #[derive(..)] +/// pub MyError, +/// +/// // Default context messages for each source error type goes below. +/// mysql::Error => "failed to interact with MySQL", +/// postgres::Error => "failed to interact with PostgreSQL", +/// opendal::Error => transparent, // if it's believed to be self-explanatory +/// // and any context is not necessary +/// } +/// ``` +/// +/// # Construction +/// +/// Unlike [`anyhow::Error`], the newtype **CANNOT** be converted from any error +/// types implicitly. Instead, it can only be converted from [`anyhow::Error`] +/// by default. +/// +/// - Users are encouraged to use [`anyhow::Context`] to attach detailed +/// information to the source error and make it an [`anyhow::Error`] before +/// converting it to the newtype. +/// +/// - Otherwise, specify the default context for each source error type as shown +/// in the example above, which will be expanded into a `From` implementation +/// from the source error type to the newtype. This should **NOT** be preferred +/// in most cases since it's less informative than the ad-hoc context provided +/// with [`anyhow::Context`] at the call site, but it could still be useful +/// during refactoring, or if the source error type is believed to be +/// self-explanatory. +/// +/// To construct a new error from scratch, one can still use macros like +/// `anyhow::anyhow!` or `risingwave_common::bail!`. Since `bail!` and `?` +/// already imply an `into()` call, developers do not need to care about the +/// type conversion most of the time. +/// +/// ## Example +/// +/// ```ignore +/// fn read_offset_from_mysql() -> Result { +/// .. +/// } +/// fn parse_offset(offset: &str) -> Result { +/// .. +/// } +/// +/// fn work() -> Result<(), MyError> { +/// // `mysql::Error` can be converted to `MyError` implicitly with `?` +/// // as the default context is provided in the definition. +/// let offset = read_offset_from_mysql()?; +/// +/// // Instead, `ParseIntError` cannot be directly converted to `MyError` +/// // with `?`, so the caller must attach context explicitly. +/// // +/// // This makes sense as the semantics of the integer ("offset") holds +/// // important information and are not implied by the error type itself. +/// let offset = parse_offset(&offset).context("failed to parse offset")?; +/// +/// if offset < 0 { +/// // Construct a new error with `bail!` macro. +/// bail!("offset `{}` must be non-negative", offset); +/// } +/// } +/// ``` +/// +/// # Discussion +/// +/// - What's the purpose of the newtype? +/// * It is to provide extra type information for errors, which makes it +/// clearer to identify which module or crate the error comes from when +/// it is passed around. +/// * It enforces the developer to attach context (explicitly or by default) +/// when doing type conversion, which makes the error more informative. +/// +/// - Is the effect essentially the same as `thiserror`? +/// * Yes, but we're here intentionally making the error type less actionable +/// to make it informative with no fear. +/// * To elaborate, consider the following `thiserror` example: +/// ```ignore +/// #[derive(thiserror::Error, Debug)] +/// pub enum MyError { +/// #[error("failed to interact with MySQL")] +/// MySql(#[from] mysql::Error), +/// #[error(transparent)] +/// Other(#[from] anyhow::Error), +/// } +/// ``` +/// This gives the caller an illusion that all errors related to MySQL are +/// under the `MySql` variant, which is not true as one could attach context +/// to an `mysql::Error` with [`anyhow::Context`] and make it go into the +/// `Other` variant. +/// +/// By doing type erasure with `anyhow`, we're making it clear that the +/// error is not actionable so such confusion is avoided. +#[macro_export] +macro_rules! def_anyhow_newtype { + (@from $error:ident transparent) => { + Self(::anyhow::Error::new($error)) + }; + (@from $error:ident $context:literal) => { + Self(::anyhow::Error::new($error).context($context)) + }; + + ( + $(#[$attr:meta])* $vis:vis $name:ident + $(, $from:ty => $context:tt)* $(,)? + ) => { + #[derive(::thiserror::Error, ::std::fmt::Debug)] + #[error(transparent)] + $(#[$attr])* $vis struct $name(#[from] #[backtrace] ::anyhow::Error); + + impl $name { + /// Unwrap the newtype to get the inner [`anyhow::Error`]. + pub fn into_inner(self) -> ::anyhow::Error { + self.0 + } + } + + $( + impl From<$from> for $name { + fn from(error: $from) -> Self { + def_anyhow_newtype!(@from error $context) + } + } + )* + }; +} diff --git a/src/error/src/lib.rs b/src/error/src/lib.rs index ccfec0cfbcc19..d3364485e8f2f 100644 --- a/src/error/src/lib.rs +++ b/src/error/src/lib.rs @@ -21,4 +21,5 @@ #![feature(register_tool)] #![register_tool(rw)] +pub mod anyhow; pub mod tonic;