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

fix(pubsublite): remove publish error translation #3843

Merged
merged 3 commits into from Mar 25, 2021

Conversation

@tmdiep
Copy link
Contributor

@tmdiep tmdiep commented Mar 20, 2021

The original intention of translating pubsublite internal errors to equivalent pubsub errors, was so that mostly the same code can be used to publish. I'd like to backtrack on this implementation decision - it wasn't explicitly discussed at any point.

The errors returned by pubsublite are wrapped to provide more useful context (e.g. the ErrOversizedMessage error includes the proto size of the user's message), so we should just propagate errors to users as-is.

The publisher clients of pubsublite and pubsub have sufficient differences in behavior, such that users will have to handle publish errors differently anyway. For example, pubsublite's PublisherClient can terminate permanently (while pubsub's Topic doesn't) and users have to create a new instance to republish failed messages.

pubsublite has a superset of pubsub errors. We will soon have an additional ErrServiceUnavailable error to indicate prolonged backend service unavailability.

@tmdiep
Copy link
Contributor Author

@tmdiep tmdiep commented Mar 20, 2021

@hongalex, @manuelmenzella-google - interested in your thoughts about this. It wasn't explicitly discussed at any point, it was just an implementation decision that I made, which I no longer think is a good idea.

@tmdiep tmdiep changed the title fix(pubsublite): remove error translation fix(pubsublite): remove publish error translation Mar 20, 2021
@manuelmenzella-google
Copy link

@manuelmenzella-google manuelmenzella-google commented Mar 22, 2021

I think this is a good change, and I like that we return more helpful messages. I do wonder, though: what do other languages (Java, Python) do here?

@tmdiep
Copy link
Contributor Author

@tmdiep tmdiep commented Mar 23, 2021

@manuelmenzella-google Java and Python currently don't have a max publish buffer size (but will as a result of b/182864157). For oversized messages, the publisher clients will receive a fatal InvalidArgument error once the message is received by the server (which checks the accounted byte size and includes this in the error message).

@manuelmenzella-google
Copy link

@manuelmenzella-google manuelmenzella-google commented Mar 25, 2021

Sounds good, I like this change. Thanks.

@tmdiep tmdiep merged commit d8d8f68 into googleapis:master Mar 25, 2021
3 checks passed
@tmdiep tmdiep deleted the untransform_errors branch Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants