Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose Error for user #1142

Open
WoWsj opened this issue Dec 6, 2023 · 8 comments
Open

Expose Error for user #1142

WoWsj opened this issue Dec 6, 2023 · 8 comments

Comments

@WoWsj
Copy link

WoWsj commented Dec 6, 2023

Is your feature request related to a problem? Please describe.
Hello, I'm currently trying to use pulser-client-go, and it seems very useful and easy.

However, producer can occur error and it is really ok. But, in some cases, I want to do some specific behavior based on its error content.

But, this package doesn't expose Error's field and doesn't offer any method or easy way to compare errors. So, I need to rewrite errors and write some comparing logic for my business logic.

I think it is not good pattern. If it is ok, please expose errors.

Describe the solution you'd like

Expose errors, and let users be able to use error.

@tisonkun
Copy link
Member

tisonkun commented Dec 7, 2023

Thanks for reporting this case @WoWsj!

I agree that currently almost all errors pulsar-client-go returns are opaque and we may expose the error in the public API or define some common categories (is_fatal? is_retriable?).

Can you share a bit more details what methods you want to handle its errors and what errors you want handle specifically?

Besides, I remember that @flowchartsman ever had a similar idea to expose client errors API in #1075. May you can share the design thoughts here to find a consensus.

Also cc @RobertIndie @gunli @Gleiphir2769 @wolfstudy @nodece

@Gleiphir2769
Copy link
Contributor

Gleiphir2769 commented Dec 7, 2023

Expose errors, and let users be able to use error.

+1. I think at least it should allow users to use the std library to compare errors (errors.Is/As).

@RobertIndie
Copy link
Member

Thanks for reporting it.

I'm +1 for this. Just to make sure we need to handle the backward compatibility carefully.

@tisonkun
Copy link
Member

tisonkun commented Dec 7, 2023

compare errors (errors.Is/As)

A rough approach is sorting out all the errors we return now and move them to a public package (pulsar/error.go) and gradually expose consts on demand (i.e., we first factor out all error "literals" to constants, but by default it's private). And put error categories a possible follow-up.

Still, user cases that need to handle concrete errors specially help us formalize the API.

@Gleiphir2769
Copy link
Contributor

I have searched all errors pulsar-client-go returned with key words fmt.Errorf and errors.New. Unfortunataly, there are almost no existing Error struct that can be exposed (e.g. errConnectionClosed). In most cases, anonymous errors are returned directly.

image image

sorting out all the errors

I think it's too difficult to sorting out ALL errors. The better way is to sort out errors relative producer/consumer public API. What do you think? @tisonkun

@nodece
Copy link
Member

nodece commented Dec 14, 2023

Thank you for your report, when you want to check if the error comes from the server or client, it isn't easy, and the error has been wrapped by fmt.Errorf(), we cannot find the original error from the wrapped error.

The better way is to sort out errors relative producer/consumer public API.

This is a good direction, and we can define a const error instead of the current implementation, and wrapping the original error by errors.Join() which is introduced by Go 1.20.

For the library, we need to be compatible with lower versions of Go, so we need to consider introducing an error-wrap library instead errors.Join(), and it needs to be compatible with errors.Is() of Go 1.20 to check the original error.

The following are known error-wrap libraries:

@WoWsj
Copy link
Author

WoWsj commented Dec 15, 2023

Still, user cases that need to handle concrete errors specially help us formalize the API.

I am trying to use pulser-client-go for streaming process application. In some cases, some errors can be handled by just printing errors(like format issues or something like just informational error), but other cases which needs specific action like recreating client needs to be handled by application side.

However, when I checked the code,

  1. Basically, errors are not exposed to users
  2. Some errors can not be exposed to user because it is only handled internally. (not sure about this 😓 )

So, In my cases, I want:

  1. Expose errors to let users easily handle it.
  2. It is good to separate errors into external errors(can be exposed to users) and internal errors(not exposed to users).

@gunli
Copy link
Contributor

gunli commented Dec 20, 2023

Thank you for your report, when you want to check if the error comes from the server or client, it isn't easy, and the error has been wrapped by fmt.Errorf(), we cannot find the original error from the wrapped error.

The better way is to sort out errors relative producer/consumer public API.

This is a good direction, and we can define a const error instead of the current implementation, and wrapping the original error by errors.Join() which is introduced by Go 1.20.

For the library, we need to be compatible with lower versions of Go, so we need to consider introducing an error-wrap library instead errors.Join(), and it needs to be compatible with errors.Is() of Go 1.20 to check the original error.

The following are known error-wrap libraries:

I have checked these packages, "https://github.com/cockroachdb/errors" require go 1.19, "https://github.com/uber-go/multierr" require go 1.20, may be we can use "https://github.com/hashicorp/go-multierror"

And, I have pushed a PR relative to this iusse #1143.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants