refactor(connector): simplify and clean-up unused variants of ConnectorError

[BugenZhao]

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

What's changed and what's your intention?

As title. See the documentation for more explanations.

Note that there were 4 error types widely used in the connector crate:

• RwError
• anyhow::Error
• SinkError
• ConnectorError

In #14950 we've migrated all RwError usages into anyhow::Error. So we still get 3.

This PR only changes the implementation of ConnectorError, which is the least used one. Following PRs may migrate anyhow::Error into it. Based on the review comments, we keep the enum structure untouched.

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.

[BugenZhao]

Current dependencies on/for this PR:

• refactor(error): eliminate usage of DisplayErrorContext #15225
• feat(lints): lint format_error on anyhow::Error #15158
• refactor(connector): migrate anyhow::Error to ConnectorError newtype #15042
• refactor(connector): make ConnectorError a wrapper of anyhow::Error #15086
• refactor(connector): simplify and clean-up unused variants of ConnectorError #15031 👈
• refactor(error): clean-up direct error formatting (part 5, done) #14956
• refactor(error): move RwError from common to frontend #14953 : 1 other dependent PR (#14954 )
• refactor(connector): replace all RwError usage with anyhow temporarily #14950
• main

This stack of pull requests is managed by Graphite.

[xxchan]

Does it mean that we lose some error context for mysql/postgres? (For Pulsar it seems ok)

[BugenZhao]

Can you elaborate more on the "context"?

[xxchan]

"Postgres error:"

[xxchan]

Previously, this is added at the conversion from tokio_postgres::Error to ConnectorError, but now this is not.

TBH I think it's not very useful. But it can be useful if tokio_postgres::Error is very vague. In this case, Postgres error: blabla is more helpful.

I think ideally we should add context() according to the higher level action we are doing though.

[BugenZhao]

I think ideally we should add context() according to the higher level action we are doing though.

Agree.

To encourage developers to provide the necessary context, a good approach would be to define ConnectorError as an enum and utilize thiserror_ext::ContextInto for conversion. 😄

This PR primarily prepares for the next one to wrap the bare anyhow::Error within ConnectorError. Once this is done, we can then consider whether switching it back to an enum implementation is appropriate.

[tabVersion]

generally LGTM, let's wait for @StrikeW 's review.

[StrikeW]

anyhow::Error is good enough if one just wants to make the error type informative but not necessarily actionable. However, given a value of type anyhow::Error, it is hard to tell which module or crate it comes from, which may blur the boundary between modules when passing it around, leading to abuse.

I think the new ConnectorError on the right side will lose the context of specific connector, e.g. MySQL error. So it is just a new type of the anyhow::Error, and doesn't embed information of specific crate/module.

Without the context information, we don't know which module throws the error and must look into the stacktrace to find out. For example, previously we can tell the IO error comes from mysql #14847.

So I -1 for current implementation, could you embed the crate/module info into the macro?

[BugenZhao]

Specifically for #14847, I believe the retrying should be done before throwing the mysql_async::Error to upper layer, which is, before turning it into a ConnectorError. This is because performing ad-hoc matching for MySQL in the general code path of handling type-erased ConnectorError appears to be an abstraction leak. In this case, the internal structure of ConnectorError is not relevant.

However, if we're intend to do that on ConnectorError, anyhow still allows developer to downcast to a concrete type, just like trait objects.

[BugenZhao]

There's a principle that errors should be either informative or actionable. In contrast of implementing the retry logic (actionable), I suppose the more reasonable part is that the refactor will lose some context in the error message (informative) and make it vaguer, as discussed in https://github.com/risingwavelabs/risingwave/pull/15031/files#r1479489349.

Specifically, the message will become:

- Connector error: MySQL error: Input/output error: Input/output error: can't parse: buf doesn't have enough data
+ Connector error: Input/output error: Input/output error: can't parse: buf doesn't have enough data

I admit that this undoubtedly make it less informative and harder to identify the root cause of external services at a glance. This could be mainly because the error messages from 3rd-party crates are not managed and reviewed by us. As a result, we are less familiar with them and could confuse them with others.

---

I'm okay to keep the original enum implementation. As pointed out by @xxchan, a prefix of MySQL error: is not as good as a manually-specified context message like Failed to read the offset from MySQL: , but could be still better than no context. We could improve that in the future.

[StrikeW]

Yeah, I am most concerning the losing context problem which would increase the burden of troubleshooting. Ideally, if we can extract 3rd party crate name into the def_anyhow_newtype macro, then we can abandon the original enum implementation.

[BugenZhao]

Based on the discussion with @xxchan, I've come up with another style of anyhow wrapper which will force the developers to provide contexts or detailed messages when upcasting the error. Will explore it in the next PRs.

[StrikeW]

I don't get how this macro solve this problem: [anyhow::Error], it is hard to tell which module or crate it comes from. It seems like defining a wrapper type of anyhow::Error.

[BugenZhao]

It seems like defining a wrapper type of anyhow::Error.

Yes, it is. I'm talking about the type-erased error type, instead of the error source type (like mysql::Error).

By wrapping anyhow::Error into ConnectorError, upper crate can clearly find it come from the connector crate and not be confused with other error types. With bare anyhow::Error, the boundary will not be clear.

[StrikeW]

LGTM, may change the PR description corresponding to new implementation.