pulsar-client-go icon indicating copy to clipboard operation
pulsar-client-go copied to clipboard

Expose Error for user

Open seongjae-min opened this issue 2 years ago • 9 comments

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.

seongjae-min avatar Dec 06 '23 05:12 seongjae-min

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 https://github.com/apache/pulsar-client-go/issues/1075. May you can share the design thoughts here to find a consensus.

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

tisonkun avatar Dec 07 '23 06:12 tisonkun

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).

Gleiphir2769 avatar Dec 07 '23 07:12 Gleiphir2769

Thanks for reporting it.

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

RobertIndie avatar Dec 07 '23 10:12 RobertIndie

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.

tisonkun avatar Dec 07 '23 10:12 tisonkun

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

Gleiphir2769 avatar Dec 14 '23 14:12 Gleiphir2769

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:

  • https://github.com/cockroachdb/errors
  • https://github.com/hashicorp/go-multierror
  • https://github.com/uber-go/multierr

nodece avatar Dec 14 '23 16:12 nodece

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).

seongjae-min avatar Dec 15 '23 01:12 seongjae-min

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:

  • https://github.com/cockroachdb/errors
  • https://github.com/hashicorp/go-multierror
  • https://github.com/uber-go/multierr

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.

gunli avatar Dec 20 '23 03:12 gunli

Can we now expose errors, and let users be able to use error? Do we have any specific function to be use to get the error? If there is, in which release if implemented.

sanketwaghmode-tibco avatar Oct 29 '24 09:10 sanketwaghmode-tibco