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

feat(pubsub): expose common errors for easier handling #7940

Merged
merged 6 commits into from May 18, 2023

Conversation

hongalex
Copy link
Member

Fixes #7925

@hongalex hongalex requested review from a team and shollyman as code owners May 15, 2023 21:36
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: pubsub Issues related to the Pub/Sub API. labels May 15, 2023
pubsub/topic.go Show resolved Hide resolved
pubsub/topic.go Outdated Show resolved Hide resolved
Copy link
Member

@codyoss codyoss left a 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")
Copy link
Member

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?

Copy link
Member Author

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.

@hongalex
Copy link
Member Author

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.

Good question. For ErrTopicStopped, that topic is fully stopped and needs to be re-initialized before calling Publish again. Otherwise, the application can't continue. I suspect that this could actually be used the code (need to record that the Publish failed in this way)

For ErrTopicOrderingNotEnabled, this is probably more informational and wouldn't require any action to be taken in the code. I'll remove this.

@hongalex
Copy link
Member Author

@codyoss @noahdietz I switched ErrPublishingPaused to a custom error that holds onto the ordering key. This leads to a slightly more elegant solution in that the string isn't awkwardly defined in two places, but now requires comparing with errors.As instead of errors.Is (since the ordering key can't be known beforehand). Thoughts on this approach instead?

Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hongalex hongalex enabled auto-merge (squash) May 18, 2023 18:00
@hongalex hongalex disabled auto-merge May 18, 2023 18:00
@hongalex hongalex merged commit 983105d into googleapis:main May 18, 2023
8 checks passed
@hongalex hongalex deleted the fix-pubsub-expose-errors branch May 18, 2023 20:23
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. size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pubsub: publish error that requires ResumePublish should wrap and return a specific exported error
3 participants