-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat(pubsub): expose common errors for easier handling #7940
Conversation
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.
Are both of these errors we expect people to handle in their code or are they informational. I would say only expose these if you expect people to check for this error and take an action in code.
pubsub/topic.go
Outdated
@@ -548,6 +548,8 @@ var errTopicStopped = errors.New("pubsub: Stop has been called for this topic") | |||
// } | |||
type PublishResult = ipubsub.PublishResult | |||
|
|||
var ErrTopicOrderingNotEnabled = errors.New("Topic.EnableMessageOrdering=false, but an OrderingKey was set in Message. Please remove the OrderingKey or turn on Topic.EnableMessageOrdering") |
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.
nit: error message should start with a lower case letter. Also other errors in this package seem to prefix with pubsub. Should we do that here too?
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.
As is, this is to maintain complete backwards compatibility (the error message is completely the same in this case). We didn't standardize on prefixing with pubsub.
I also struggled with fixing this for ErrPublishingPaused
, which starts in the middle of the previous error message to allow for error wrapping.
Good question. For For |
@codyoss @noahdietz I switched |
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
Fixes #7925