-
Notifications
You must be signed in to change notification settings - Fork 336
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
[Improve][Producer] normalize and export the errors #1143
Conversation
Makefile
Outdated
@@ -20,7 +20,7 @@ | |||
IMAGE_NAME = pulsar-client-go-test:latest | |||
PULSAR_VERSION ?= 2.10.3 | |||
PULSAR_IMAGE = apachepulsar/pulsar:$(PULSAR_VERSION) | |||
GO_VERSION ?= 1.18 | |||
GO_VERSION ?= 1.20 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good to me.
This is a major change. Could you upgrade the Go version in another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RobertIndie If I upgrade it in another PR, this PR will fail the CI, and the CI is failed now, may be I should update the go version in ci.yml either:
go-version: [1.16, 1.17, 1.18, 1.19]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break the CI and will break the compatibility for the older go version. Could you implement it another way? Or do you think there is a strong motivation that we need to upgrade the go version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't mean to upgrade the go version, it just so happends that I use go 1.20 at the same time, I implement this is PR just in the purpose of retrying and when I test my retrying demo and found that I can not retry all the failed requests, some errors should fail fast instead of retrying(.e.g ErrSendQueueIsFull).
And I just found #1142 today, and found that this is a common requirement. I've checked the 3rd party packages, "https://github.com/cockroachdb/errors" require go 1.19, "https://github.com/hashicorp/go-multierror" has a different package name multierror
, "https://github.com/uber-go/multierr" require go 1.20.
For the sake of compatibility, may be the simplest way is to implement errors.Join() in pulsar-client-go, what I mean is to copy the code in go SDK into our package, I will try that tomorrow.
@RobertIndie I have updated this PR and use "github.com/hashicorp/go-multierror" to join errors instead of errors.Join() of Go 1.20, would you PTAL at again? Also cc @tisonkun @nodece @WoWsj! @Gleiphir2769 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
(If this PR fixes a github issue, please add
Fixes #<xyz>
.)Fixes #
(or if this PR is one task of a github issue, please add
Master Issue: #<xyz>
to link to the master issue.)Master Issue: #
Motivation
Currently, producing retrying is not implemented by the client, when I want to retry, it depends on the errors returned by the callback, some errors can retry, some should stop and fail fast(for example the queue is full), but the errors are not normalized and exported now, this PR normalize and export the errors of the producer.
Modifications
joinErrors(errs ...errors)
to join errors:errors.Is(err, pulsar.ErrChema)
to determine if an error can retry or not;Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If
yes
was chosen, please highlight the changesDocumentation