refactor(error): clean-up direct error formatting (part 1) #13763

BugenZhao · 2023-12-01T08:42:21Z

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

What's changed and what's your intention?

See backgrounds in #11443. TL;DR: formatting errors with {} or .to_string can cause the loss of the error source. Use thiserror_ext::Report instead.

Empowered by the lints introduced in #13750.

Run cargo install cargo-dylint dylint-link, then add the following entry to .vscode/settings.json to find all places to fix:

"rust-analyzer.check.overrideCommand": [
    "cargo",
    "dylint",
    "--all",
    "--workspace",
    "--",
    "--all-targets",
    "--message-format=json"
]

This PR fixes

common
batch
stream
compute

and some minor and related ones.

The remainings are...

connector
ctl
expr
frontend
jni
meta
storage (hummock, object store)
risedev
rpc client
pgwire

Checklist

I have written necessary rustdoc comments
I have added necessary unit tests and integration tests
I have added test labels as necessary. See details.
I have added fuzzing tests or opened an issue to track them. (Optional, recommended for new SQL features Sqlsmith: Sql feature generation #7934).
My PR contains breaking changes. (If it deprecates some features, please create a tracking issue to remove them in the future).
All checks passed in ./risedev check (or alias, ./risedev c)
My PR changes performance-critical code. (Please run macro/micro-benchmarks and show the results.)

My PR contains critical fixes that are necessary to be merged into the latest release. (Please check out the details)

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 <i@bugenzhao.com>

BugenZhao

Some guides for refactoring

BugenZhao · 2023-12-04T07:29:15Z

src/batch/src/executor/aggregation/orderby.rs

-                .map_err(|e| ExprError::Internal(anyhow!("failed to encode row, error: {}", e)))?;
+                .context("failed to encode row")?;


Use Anyhow::Context::context to create a new internal error with the given message, while keeping the error source.

BugenZhao · 2023-12-04T07:30:09Z

src/common/src/config.rs

@@ -101,8 +101,11 @@ where
        RwConfig::default()
    } else {
        let config_str = fs::read_to_string(path)
-            .unwrap_or_else(|e| panic!("failed to open config file '{}': {}", path, e));
-        toml::from_str(config_str.as_str()).unwrap_or_else(|e| panic!("parse error {}", e))
+            .with_context(|| format!("failed to open config file at `{path}`"))


Use with_context if evaluating the message is not const.

BugenZhao · 2023-12-04T07:32:07Z

src/batch/src/executor/insert.rs

@@ -231,8 +231,8 @@ impl BoxedExecutorBuilder for InsertExecutor {
                .map(|IndexAndExpr { index: i, expr: e }| {
                    Ok((
                        i as usize,
-                        build_from_prost(&e.ok_or_else(|| anyhow!("expression is None"))?)
-                            .map_err(|e| anyhow!("failed to build expression: {}", e))?,
+                        build_from_prost(&e.context("expression is None")?)


Besides Result, context is also applicable to Options.

If you only have the error, use anyhow::anyhow!(source).context("context message").

BugenZhao · 2023-12-04T07:33:03Z

src/batch/src/rpc/service/task_service.rs

-        let mut output = task.get_task_output(&pb_task_output_id).map_err(|e| {
+        let mut output = task.get_task_output(&pb_task_output_id).inspect_err(|e| {
            error!(
-                "failed to get task output of Task {:?} in local execution mode",
-                task_id
+                error = %e.as_report(),
+                ?task_id,
+                "failed to get task output in local execution mode",
            );
-            e
        })?;


Prefer inspect_err if not going to change the error variant. For example, logging it out.

BugenZhao · 2023-12-04T07:34:48Z

src/batch/src/rpc/service/task_service.rs

            error!(
-                "failed to build executors and trigger execution of Task {:?}: {}",
-                task_id, e
+                error = %e.as_report(),
+                ?task_id,
+                "failed to build executors and trigger execution"
            );


To print an error with tracing, specify a field with error = %e.as_report(). Explanations:

Use the Report wrapper for error to visit the source chain when visiting.

Telling tracing to use Display format with %.

Normalize the error field name to error with error = <expr>

BugenZhao · 2023-12-04T07:36:34Z

src/batch/src/task/task_execution.rs

@@ -671,7 +672,7 @@ impl<C: BatchTaskContext> BatchTaskExecution<C> {

        let error = error.map(Arc::new);
        *self.failure.lock() = error.clone();
-        let err_str = error.as_ref().map(|e| e.to_string());
+        let err_str = error.as_ref().map(|e| e.to_report_string());


If we really want to eagerly format the error, use to_report_string to replace to_string. This is basically an alias of e.as_report().to_string().

Note that to_string implies Display trait. The behavior of {} {:#} {:?} {:#?} differ, and there're also corresponding to_report_string variants. Check the documentation of Report.

BugenZhao · 2023-12-04T07:41:35Z

src/compute/src/rpc/service/config_service.rs

-            .map_err(|e| tonic::Status::internal(format!("{}", e)))?;
+            .map_err(|e| e.to_status(Code::Internal, "compute"))?;
        let stream_config = serde_json::to_string(&self.stream_mgr.config().await)
-            .map_err(|e| tonic::Status::internal(format!("{}", e)))?;
+            .map_err(|e| e.to_status(Code::Internal, "compute"))?;


Use to_status to convert an error to tonic::Status. If this is not ad-hoc, you may want to directly impl From<..> for tonic::Status.

BugenZhao · 2023-12-04T07:43:33Z

src/error/src/tonic.rs

@@ -145,6 +146,7 @@ impl std::fmt::Display for TonicStatusWrapper {
            write!(f, " to {} service", service_name)?;
        }
        write!(f, " failed: {}: ", self.0.code())?;
+        #[allow(rw::format_error)] // intentionally format the source itself


Ignore the customized lints just like how we do for clippy.

Suggested change

#[allow(rw::format_error)] // intentionally format the source itself

#[expect(rw::format_error)] // intentionally format the source itself

BugenZhao · 2023-12-04T07:44:11Z

src/compute/src/server.rs

+                                if let Err(_err) = shutdown_sender.send(()) {
+                                    tracing::warn!("Failed to send shutdown");


No need to print the SendError if the value is meaningless, as it does not contain useful information.

Note: the value is the data not sent

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

src/common/common_service/src/observer_manager.rs

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

src/common/src/types/mod.rs

src/compute/src/rpc/service/config_service.rs

src/connector/src/lib.rs

xxchan · 2023-12-04T08:45:42Z

src/stream/src/error.rs

-#[derive(thiserror::Error, Debug, thiserror_ext::Box)]
+#[derive(
+    thiserror::Error,
+    Debug,
+    thiserror_ext::Arc,
+    thiserror_ext::ContextInto,
+    thiserror_ext::Construct,
+)]


Why Arc

What is ContextInto

Why Arc

The error is cloned here:

risingwave/src/stream/src/task/barrier_manager/managed_state.rs

Lines 179 to 181 in f5efe3f

&& collect_notifier

.send(Err(err.clone()))

.is_err()

What is ContextInto

It's another kind of constructor derivation! 😄 Basically, we can write

risingwave/src/stream/src/task/barrier_manager/managed_state.rs

Lines 167 to 168 in f5efe3f

// Attach the actor id to the error.

let err = err.into_unexpected_exit(actor_id);

as a sugar of

let err = StreamError::unexpected_exit(err, actor_id);

It's not that sweet here, but can be useful for transforming the Results as it's also implemented on Results.

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

Lines 49 to 52 in f5efe3f

output.status.exit_ok().into_exit_error(

String::from_utf8_lossy(&output.stdout),

String::from_utf8_lossy(&output.stderr),

)?;

It's highly inspired by anyhow::Context and snafu::Context. I'll write the rustdoc for it ASAP.

Oh, the context here is always evaluated. 😕 Perhaps I need to introduce a with method.

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

Lines 49 to 52 in f5efe3f

output.status.exit_ok().into_exit_error(

String::from_utf8_lossy(&output.stdout),

String::from_utf8_lossy(&output.stderr),

)?;

xxchan

Others LGTM. 🫡 for the work. But I'd be even happier if the PR can be even smaller.

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

xxchan · 2023-12-04T09:50:22Z

.cargo/config.toml

-    # uncomment the following two lines to enable `TaskLocalAlloc`
+
+    # Register the `rw` tool for referencing the custom lints in attributes like `#[allow(rw::xx)]`.
+    # This is essentially the same as `#![..]` in each crate root.


Oh, can we use this to enable all rust features? 🫣

Yeah, really interesting! But it seems painful to also override RUSTDOCFLAGS. 😕

Seems not to work for doc tests. I'll manually register it. 🤡

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

codecov · 2023-12-05T08:40:40Z

Codecov Report

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

Comparison is base (a73d940) 68.19% compared to head (4a99055) 68.26%.
Report is 16 commits behind head on main.

Files	Patch %	Lines
src/utils/runtime/src/logger.rs	0.00%	13 Missing ⚠️
src/stream/src/cache/managed_lru.rs	0.00%	10 Missing ⚠️
src/stream/src/task/barrier_manager.rs	9.09%	10 Missing ⚠️
src/common/src/array/proto_reader.rs	67.85%	9 Missing ⚠️
src/batch/src/rpc/service/task_service.rs	0.00%	8 Missing ⚠️
src/common/src/array/arrow.rs	33.33%	6 Missing ⚠️
src/common/src/types/mod.rs	66.66%	6 Missing ⚠️
src/compute/src/rpc/service/stream_service.rs	0.00%	6 Missing ⚠️
...c/stream/src/task/barrier_manager/managed_state.rs	0.00%	6 Missing ⚠️
src/common/src/config.rs	0.00%	5 Missing ⚠️
... and 33 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #13763      +/-   ##
==========================================
+ Coverage   68.19%   68.26%   +0.06%     
==========================================
  Files        1524     1524              
  Lines      262232   262275      +43     
==========================================
+ Hits       178829   179033     +204     
+ Misses      83403    83242     -161

Flag	Coverage Δ
rust	`68.26% <34.21%> (+0.06%)`	⬆️

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.

github-actions bot added the type/refactor label Dec 1, 2023

Base automatically changed from bz/dylint-checkin to main December 1, 2023 09:12

BugenZhao added 5 commits December 1, 2023 18:09

fix for batch

739980a

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

ignore in test code

3ceba5a

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

fix for common

33794c8

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

auto register rw tool in rustflags

74fbfbd

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

fix for compute and some of stream

b3603f9

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

BugenZhao force-pushed the bz/format-error-lint-fix branch from 1d92df9 to b3603f9 Compare December 1, 2023 10:10

BugenZhao added ci/run-build ci/run-build-simulation labels Dec 1, 2023

BugenZhao added 3 commits December 1, 2023 20:28

fix arrow error

6c4deb1

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

Merge remote-tracking branch 'origin/main' into bz/format-error-lint-fix

dee6e9d

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

refine actor exit unexpectedly

30b8172

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

BugenZhao force-pushed the bz/format-error-lint-fix branch from 4dd80b0 to 30b8172 Compare December 4, 2023 07:18

BugenZhao added 2 commits December 4, 2023 15:25

refine attrs

6478283

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

minor tweaks

28a6aa1

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

BugenZhao commented Dec 4, 2023

View reviewed changes

BugenZhao removed type/refactor ci/run-build ci/run-build-simulation labels Dec 4, 2023

BugenZhao marked this pull request as ready for review December 4, 2023 07:56

BugenZhao requested a review from a team as a code owner December 4, 2023 07:56

fix remaining streams

b22bb2f

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

xxchan reviewed Dec 4, 2023

View reviewed changes

src/common/common_service/src/observer_manager.rs Show resolved Hide resolved

github-actions bot added the type/refactor label Dec 4, 2023

BugenZhao added 4 commits December 4, 2023 16:11

tweak attributes

57916bf

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

fix for runtime logger

c4f3d3b

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

fix cargo sort

25a7cb9

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

fix clippy

f5efe3f

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

xxchan reviewed Dec 4, 2023

View reviewed changes

src/common/src/types/mod.rs Show resolved Hide resolved

xxchan reviewed Dec 4, 2023

View reviewed changes

src/compute/src/rpc/service/config_service.rs Show resolved Hide resolved

BugenZhao mentioned this pull request Dec 4, 2023

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

Open

xxchan reviewed Dec 4, 2023

View reviewed changes

src/connector/src/lib.rs Outdated Show resolved Hide resolved

BugenZhao requested review from TennyZhuang, wangrunji0408 and ZENOTME December 4, 2023 08:36

xxchan reviewed Dec 4, 2023

View reviewed changes

BugenZhao requested a review from yuhao-su December 4, 2023 08:47

xxchan approved these changes Dec 4, 2023

View reviewed changes

BugenZhao added 3 commits December 4, 2023 17:24

fix doc build

5679ace

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

retain addr parse context

fcaa579

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

change some allow to expect

60bd791

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

xxchan reviewed Dec 4, 2023

View reviewed changes

BugenZhao added 5 commits December 4, 2023 17:50

fix flag for doctest

a2d11cb

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

avoid using Debug format in thiserror display

3e3bcbc

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

print the backtrace when actor exit

0a3c47b

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

manually allow rw tool

7721ad8

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

add vscode config example

4a99055

Signed-off-by: Bugen Zhao <i@bugenzhao.com>

BugenZhao enabled auto-merge December 5, 2023 08:46

BugenZhao added this pull request to the merge queue Dec 5, 2023

stdrc approved these changes Dec 5, 2023

View reviewed changes

Merged via the queue into main with commit 4f90e37 Dec 5, 2023

BugenZhao deleted the bz/format-error-lint-fix branch December 5, 2023 09:53

yuhao-su mentioned this pull request Dec 8, 2023

refactor(error): clean-up direct error formatting (part 2) #13870

Merged

9 tasks

This was referenced Jan 18, 2024

refactor: avoid RwError in schema registry client #14603

Merged

refactor(error): clean-up direct error formatting (part 3) #14685

Merged

refactor(error): clean-up direct error formatting (part 4) #14691

Merged

KeXiangWang mentioned this pull request Apr 23, 2024

fix(cdc): fix enum to varchar in postgres-cdc #16423

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(error): clean-up direct error formatting (part 1) #13763

refactor(error): clean-up direct error formatting (part 1) #13763

BugenZhao commented Dec 1, 2023 •

edited

Loading

BugenZhao left a comment

BugenZhao Dec 4, 2023

BugenZhao Dec 4, 2023

BugenZhao Dec 4, 2023 •

edited

Loading

BugenZhao Dec 4, 2023

BugenZhao Dec 4, 2023 •

edited

Loading

BugenZhao Dec 4, 2023

BugenZhao Dec 4, 2023

BugenZhao Dec 4, 2023

xxchan Dec 4, 2023

BugenZhao Dec 4, 2023

xxchan Dec 4, 2023

xxchan Dec 4, 2023

BugenZhao Dec 4, 2023 •

edited

Loading

BugenZhao Dec 4, 2023

xxchan left a comment

xxchan Dec 4, 2023 •

edited

Loading

BugenZhao Dec 4, 2023

BugenZhao Dec 5, 2023

codecov bot commented Dec 5, 2023 •

edited

Loading

		.map_err(\|e\| ExprError::Internal(anyhow!("failed to encode row, error: {}", e)))?;
		.context("failed to encode row")?;

	#[allow(rw::format_error)] // intentionally format the source itself
	#[expect(rw::format_error)] // intentionally format the source itself

		if let Err(_err) = shutdown_sender.send(()) {
		tracing::warn!("Failed to send shutdown");

	// Attach the actor id to the error.
	let err = err.into_unexpected_exit(actor_id);

	output.status.exit_ok().into_exit_error(
	String::from_utf8_lossy(&output.stdout),
	String::from_utf8_lossy(&output.stderr),
	)?;

refactor(error): clean-up direct error formatting (part 1) #13763

refactor(error): clean-up direct error formatting (part 1) #13763

Conversation

BugenZhao commented Dec 1, 2023 • edited Loading

What's changed and what's your intention?

Checklist

Documentation

Release note

BugenZhao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BugenZhao Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BugenZhao Dec 4, 2023 • edited Loading

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 Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxchan left a comment

Choose a reason for hiding this comment

xxchan Dec 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 5, 2023 • edited Loading

Codecov Report

BugenZhao commented Dec 1, 2023 •

edited

Loading

BugenZhao Dec 4, 2023 •

edited

Loading

BugenZhao Dec 4, 2023 •

edited

Loading

BugenZhao Dec 4, 2023 •

edited

Loading

xxchan Dec 4, 2023 •

edited

Loading

codecov bot commented Dec 5, 2023 •

edited

Loading