refactor(error): eliminate most `RwError` usages in `common` crate #13588

BugenZhao · 2023-11-22T08:13:58Z

I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.

What's changed and what's your intention?

Eliminate most RwError usages in the common crate. #4077

To achieve that, one must not hesitate to define per-module error types. Examples:

risingwave/src/common/heap_profiling/src/jeprof.rs

Lines 20 to 32 in 05ab5e1

    
           #[derive(thiserror::Error, Debug, thiserror_ext::ContextInto)] 
        
           pub enum JeprofError { 
        
               #[error(transparent)] 
        
               IoError(#[from] std::io::Error), 
        
               #[error("jeprof exit with an error (stdout: {stdout}, stderr: {stderr}): {inner}")] 
        
               ExitError { 
        
                   #[source] 
        
                   inner: std::process::ExitStatusError, 
        
                   stdout: String, 
        
                   stderr: String, 
        
               }, 
        
           }

risingwave/src/common/src/types/postgres_type.rs

Lines 51 to 53 in 05ab5e1

    
           #[derive(Debug, thiserror::Error)] 
        
           #[error("Unsupported oid {0}")] 
        
           pub struct UnsupportedOid(i32);

The remaining ones are:

adaptors with ArrayError
session_config, which is conflict with refactor(frontend): config session variable using proc-macro #13554
the error definition itself

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

My PR needs documentation updates. (Please use the Release note section below to summarize the impact on users)

Release note

If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.

Signed-off-by: Bugen Zhao <[email protected]>

codecov · 2023-11-22T08:53:18Z

Codecov Report

Attention: 101 lines in your changes are missing coverage. Please review.

Comparison is base (bf3975f) 68.18% compared to head (b87318c) 68.13%.
Report is 3 commits behind head on main.

Files	Patch %	Lines
src/common/src/types/mod.rs	27.90%	31 Missing ⚠️
src/common/src/types/interval.rs	56.00%	11 Missing ⚠️
src/common/common_service/src/observer_manager.rs	23.07%	10 Missing ⚠️
src/common/heap_profiling/src/jeprof.rs	0.00%	9 Missing ⚠️
src/common/src/types/to_binary.rs	11.11%	8 Missing ⚠️
src/common/src/types/num256.rs	0.00%	4 Missing ⚠️
...storage/hummock_test/src/bin/replay/replay_impl.rs	0.00%	4 Missing ⚠️
src/common/src/error.rs	0.00%	3 Missing ⚠️
src/common/src/types/datetime.rs	0.00%	3 Missing ⚠️
src/utils/pgwire/src/pg_protocol.rs	25.00%	3 Missing ⚠️
... and 13 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13588      +/-   ##
==========================================
- Coverage   68.18%   68.13%   -0.05%     
==========================================
  Files        1520     1519       -1     
  Lines      261483   261375     -108     
==========================================
- Hits       178289   178097     -192     
- Misses      83194    83278      +84

Flag	Coverage Δ
rust	`68.13% <48.20%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wangrunji0408

LGTM!

Signed-off-by: Bugen Zhao <[email protected]>

BugenZhao · 2023-11-24T06:13:36Z

Update session config error in b87318c. cc @yuhao-su

BugenZhao · 2023-11-24T06:23:27Z

Update session config error in b87318c.

The error message is much more readable now!

dev=> set rw_implicit_flush to maybe;
ERROR:  Failed to run the query

Caused by these errors (recent errors listed first):
  1: Failed to get/set session config
  2: Invalid value `maybe` for `rw_implicit_flush`
  3: provided string was not `true` or `false`

dev=> set streaming_parallelism to 888888888888888888888888;
ERROR:  Failed to run the query

Caused by these errors (recent errors listed first):
  1: Failed to get/set session config
  2: Invalid value `888888888888888888888888` for `streaming_parallelism`
  3: number too large to fit in target type

dev=> set isolation_level to hello;
ERROR:  Failed to run the query

Caused by these errors (recent errors listed first):
  1: Failed to get/set session config
  2: Unrecognized config entry `isolation_level`

dev=> set transaction_isolation_level to hello;
ERROR:  Failed to run the query

Caused by these errors (recent errors listed first):
  1: Failed to get/set session config
  2: Invalid value `hello` for `transaction_isolation_level`
  3: isolation level is not yet supported

stdrc

LGTM! Thanks for your elegant work! Could you please write something like an error-handling guide?

stdrc · 2023-11-24T06:21:36Z

src/common/heap_profiling/src/jeprof.rs

+        #[source]
+        inner: std::process::ExitStatusError,
+        stdout: String,
+        stderr: String,


#[source] will generate From<ExitStatusError> for JeprofError right? But how are stdout and stderr set?

Does this work because of thiserror_ext::ContextInto which generates into_exit_error which will retain the source error as inner and set the arguments for stdout and stderr?

Does this work because of thiserror_ext::ContextInto which generates into_exit_error which will retain the source error as inner and set the arguments for stdout and stderr?

Exactly! I'll refine the documentation of thiserror_ext once it gets more stable.

#[source] will generate From<ExitStatusError> for JeprofError right?

This is not accurate. Only #[from] will generate From impl, while #[source] only helps us to correctly maintain the source chain with source() method.

stdrc · 2023-11-24T06:23:38Z

src/common/src/catalog/column.rs

-    /// Find `column_desc` in `field_descs` by name.
-    pub fn field(&self, name: &String) -> crate::error::Result<(ColumnDesc, i32)> {
-        if let DataType::Struct { .. } = self.data_type {
-            for (index, col) in self.field_descs.iter().enumerate() {
-                if col.name == *name {
-                    return Ok((col.clone(), index as i32));
-                }
-            }
-            Err(ErrorCode::ItemNotFound(format!("Invalid field name: {}", name)).into())
-        } else {
-            Err(ErrorCode::ItemNotFound(format!(
-                "Cannot get field from non nested column: {}",
-                self.name
-            ))
-            .into())
-        }
-    }
-


This is a dead method?

Yes. I don't find any references.

stdrc · 2023-11-24T06:24:51Z

src/common/src/catalog/mod.rs

@@ -136,7 +136,7 @@ pub fn cdc_table_name_column_desc() -> ColumnDesc {
 /// The local system catalog reader in the frontend node.
 #[async_trait]
 pub trait SysCatalogReader: Sync + Send + 'static {
-    async fn read_table(&self, table_id: &TableId) -> Result<Vec<OwnedRow>>;
+    async fn read_table(&self, table_id: &TableId) -> Result<Vec<OwnedRow>, BoxedError>;


Will BoxedError be abused after removing RwError?

Since this is a trait, we have no information about how the users may have implemented it, so does the error type. It seems a common practice to let trait methods return Box<dyn Error>. Similar ideas are applied to sqllogictest::AsyncDB.

stdrc · 2023-11-24T06:27:45Z

src/common/src/types/interval.rs

@@ -1001,6 +1000,24 @@ impl ToText for crate::types::Interval {
    }
 }

+/// Error type for parsing an [`Interval`].
+#[derive(thiserror::Error, Debug, thiserror_ext::Construct)]


Is there any big difference between IntervalParseError::Invalid and IntervalParseError::invalid? Given that the former one is also a special function?

One can pass any impl Into<String> with constructors.

Also, if there's a backtrace field, it can also be automatically generated.

If there's a newtype wrapper (error-handling: simplify boxed error definition #13215), it'll be directly implemented on the wrapper type.

#[automatically_derived] impl IntervalParseError { /// Constructs a [`IntervalParseError::Invalid`] variant. pub fn invalid(arg_0: impl Into<String>) -> Self { IntervalParseError::Invalid { 0: arg_0.into() }.into() } }

fuyufjh

LGTM

yuhao-su

LGTM🐮

xxchan

I often forget why on earth do we want to eliminate big RwError. Do you have a tldr? 🤪

What's the largest benefit?

BugenZhao · 2023-11-29T03:54:26Z

I often forget why on earth do we want to eliminate big RwError. Do you have a tldr? 🤪

What's the largest benefit?

The worst part of RwError is that it now acts as both the leaf error type (that needs to be frequently and manually created by developers #13627) and the root error type (that returns to pgwire).

These two roles are not that compatible. For example, the leaf error usually contains a message field only, while the root error is likely to be simply some #[from] wrappers with contexts attached. As a result, we get BindError and BindErrorRoot. It's apparent that a better practice should be only keep the Root variant and make BindError a separate error type.

risingwave/src/common/src/error.rs

Lines 151 to 161 in b16bbcc

    
           // TODO: use a new type for bind error 
        
           #[error("Bind error: {0}")] 
        
           BindError(String), 
        
           // TODO: only keep this one 
        
           #[error("Failed to bind expression: {expr}: {error}")] 
        
           BindErrorRoot { 
        
               expr: String, 
        
               #[source] 
        
               #[backtrace] 
        
               error: BoxedError, 
        
           },

There're also other reasons for removing RwError...

With per-module error types, developers are freer to design the types so the categorization can be more accurate.
Different modules may expect different boxing or backtrace behavior of its error type.
By removing that, it's also a good chance for us to revisit our previous error handling practice. 😄

BugenZhao added 2 commits November 22, 2023 15:09

refactor some

b1c0b3e

Signed-off-by: Bugen Zhao <[email protected]>

refactor more

05ab5e1

Signed-off-by: Bugen Zhao <[email protected]>

BugenZhao requested review from wangrunji0408, yuhao-su, xxchan and ZENOTME November 22, 2023 08:13

BugenZhao requested a review from a team as a code owner November 22, 2023 08:13

github-actions bot added the type/refactor label Nov 22, 2023

add documentation

971f77c

Signed-off-by: Bugen Zhao <[email protected]>

BugenZhao requested a review from fuyufjh November 23, 2023 07:51

wangrunji0408 approved these changes Nov 24, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into bz/error-rw-common

1c3a538

Signed-off-by: Bugen Zhao <[email protected]>

BugenZhao force-pushed the bz/error-rw-common branch from a1eaf84 to 1c3a538 Compare November 24, 2023 05:17

refine session config error

b87318c

Signed-off-by: Bugen Zhao <[email protected]>

BugenZhao enabled auto-merge November 24, 2023 06:25

BugenZhao added this pull request to the merge queue Nov 24, 2023

stdrc reviewed Nov 24, 2023

View reviewed changes

stdrc approved these changes Nov 24, 2023

View reviewed changes

fuyufjh approved these changes Nov 24, 2023

View reviewed changes

yuhao-su reviewed Nov 24, 2023

View reviewed changes

yuhao-su approved these changes Nov 24, 2023

View reviewed changes

Merged via the queue into main with commit 90ce699 Nov 24, 2023
27 of 28 checks passed

BugenZhao deleted the bz/error-rw-common branch November 24, 2023 07:14

BugenZhao mentioned this pull request Nov 24, 2023

refactor(error): simplify leaf error instantiation #13627

Merged

4 tasks

xxchan reviewed Nov 29, 2023

View reviewed changes

This was referenced Nov 29, 2023

Tracking: Eliminate usages of RwError #4077

Closed

refactor: reinvestigate error handling and reporting in the system #11443

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(error): eliminate most `RwError` usages in `common` crate #13588

refactor(error): eliminate most `RwError` usages in `common` crate #13588

BugenZhao commented Nov 22, 2023 •

edited

Loading

codecov bot commented Nov 22, 2023 •

edited

Loading

wangrunji0408 left a comment

BugenZhao commented Nov 24, 2023 •

edited

Loading

BugenZhao commented Nov 24, 2023

stdrc left a comment

stdrc Nov 24, 2023

BugenZhao Nov 24, 2023

BugenZhao Nov 24, 2023

stdrc Nov 24, 2023

BugenZhao Nov 24, 2023

stdrc Nov 24, 2023

BugenZhao Nov 24, 2023

stdrc Nov 24, 2023

BugenZhao Nov 24, 2023 •

edited

Loading

fuyufjh left a comment

yuhao-su left a comment

xxchan left a comment •

edited

Loading

BugenZhao commented Nov 29, 2023 •

edited

Loading

	#[derive(thiserror::Error, Debug, thiserror_ext::ContextInto)]
	pub enum JeprofError {
	#[error(transparent)]
	IoError(#[from] std::io::Error),

	#[error("jeprof exit with an error (stdout: {stdout}, stderr: {stderr}): {inner}")]
	ExitError {
	#[source]
	inner: std::process::ExitStatusError,
	stdout: String,
	stderr: String,
	},
	}

	#[derive(Debug, thiserror::Error)]
	#[error("Unsupported oid {0}")]
	pub struct UnsupportedOid(i32);

refactor(error): eliminate most RwError usages in common crate #13588

refactor(error): eliminate most RwError usages in common crate #13588

Conversation

BugenZhao commented Nov 22, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

codecov bot commented Nov 22, 2023 • edited Loading

Codecov Report

wangrunji0408 left a comment

Choose a reason for hiding this comment

BugenZhao commented Nov 24, 2023 • edited Loading

BugenZhao commented Nov 24, 2023

stdrc 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

BugenZhao Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

fuyufjh left a comment

Choose a reason for hiding this comment

yuhao-su left a comment

Choose a reason for hiding this comment

xxchan left a comment • edited Loading

Choose a reason for hiding this comment

BugenZhao commented Nov 29, 2023 • edited Loading

refactor(error): eliminate most `RwError` usages in `common` crate #13588

refactor(error): eliminate most `RwError` usages in `common` crate #13588

BugenZhao commented Nov 22, 2023 •

edited

Loading

codecov bot commented Nov 22, 2023 •

edited

Loading

BugenZhao commented Nov 24, 2023 •

edited

Loading

BugenZhao Nov 24, 2023 •

edited

Loading

xxchan left a comment •

edited

Loading

BugenZhao commented Nov 29, 2023 •

edited

Loading