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

pubsub: publish error that requires ResumePublish should wrap and return a specific exported error #7925

Closed
ramirezalbert3 opened this issue May 15, 2023 · 3 comments · Fixed by #7940
Assignees
Labels
api: pubsub Issues related to the Pub/Sub API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@ramirezalbert3
Copy link

Is your feature request related to a problem? Please describe.

When publishing with an ordering key you may end up in a situation where you're required to ResumePublish because the scheduler was paused. This would be much easier to detect and handle if instead of:

err = fmt.Errorf("pubsub: Publishing for ordering key, %s, paused due to previous error. Call topic.ResumePublish(orderingKey) before resuming publishing", orderingKey)

we had something like:

// first
var ErrPublishingPaused = errors.New("Call topic.ResumePublish(orderingKey) before resuming publishing")
// and then
err = fmt.Errorf("pubsub: Publishing for ordering key, %s, paused due to previous error. %w", orderingKey, ErrPublishingPaused)

Describe the solution you'd like
In the example above I believe there's a good enough, fully backwards compatible and idiomatic solution. Similar implementations could be proposed that don't rely on errors.New if needed.

@ramirezalbert3 ramirezalbert3 added the triage me I really want to be triaged. label May 15, 2023
@product-auto-label product-auto-label bot added the api: pubsub Issues related to the Pub/Sub API. label May 15, 2023
@hongalex hongalex added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed triage me I really want to be triaged. labels May 15, 2023
@hongalex
Copy link
Member

This would be much easier to detect and handle

Could you elaborate more on why you need the distinction between the first or subsequent failures for ordering keys?

fully backwards compatible

I have a concern that since we are changing the error message here, it's not entirely backwards compatible. Ideally, users should not be doing error string comparisons, but since error wrapping was introduced in a Go version that wasn't supported when this bit was written, I suspect that might be what's happening. Might still be worth doing, but would require more specific justification.

@ramirezalbert3
Copy link
Author

Sorry maybe I didn't explain myself properly, what we'd need is for a way to be able to distinguish this error situation that requires calling ResumePublish from other errors that may not require it? Example here: https://cloud.google.com/pubsub/docs/samples/pubsub-resume-publish-with-ordering-keys

About backwards compatibility, fully agree that should be done preserving the same string representation, i tried to go for it with my example?

@hongalex
Copy link
Member

Ah gotcha, I think I misunderstood to thinking you wanted different error messages for the first occurrence of a failed ordering message.

Adding a well defined (wrapped) error shouldn't be breaking and should be fine. I can throw together a PR for this. Thanks for the suggestion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants