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): Allow Message and PublishResult to be used outside the package #3200

Merged
merged 2 commits into from Nov 13, 2020

Conversation

@tmdiep
Copy link
Contributor

@tmdiep tmdiep commented Nov 12, 2020

  • Refactored ack/nack handling to an ackHandler interface. Added NewMessage() for external packages to create messages with a custom ack handler.
    • Refactored usage inside the pubsub package.
  • Added NewPublishResult() to expose set() as a func. Users will not be able to inadvertently call this to set a publish result before it is ready.
@tmdiep tmdiep requested a review from hongalex Nov 12, 2020
@tmdiep tmdiep requested a review from as a code owner Nov 12, 2020
@google-cla google-cla bot added the cla: yes label Nov 12, 2020
Copy link
Member

@hongalex hongalex left a comment

LGTM, thanks!

Loading

@hongalex
Copy link
Member

@hongalex hongalex commented Nov 13, 2020

For context, this PR was opened after discussing the best way to maintain compatibility with the new Pub/Sub Lite Go library. Prior to this change, ack/nacks are not exposed which meant that any code outside this package wouldn't be able to reuse pubsub.Message. This PR exposes the relevant bits of Message and PublishResult with constructors. Although this increases the API surface, I believe this is the best path forward. This change has the adsded benefit of making the pubsub package easier to test (originally proposed in #2920)

Loading

@tmdiep tmdiep merged commit 581bf92 into googleapis:master Nov 13, 2020
3 checks passed
Loading
@tmdiep tmdiep deleted the pubsub branch Nov 13, 2020

// NewMessage creates a message with a custom ack/nack handler, which should not
// be nil.
func NewMessage(ackh ackHandler) *Message {
Copy link
Contributor

@dsymonds dsymonds Nov 16, 2020

Choose a reason for hiding this comment

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

This is a poor API. It'll look weird in package docs since ackHandler is unexported.

Loading

Copy link
Member

@hongalex hongalex Nov 16, 2020

Choose a reason for hiding this comment

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

I don't see why ackHandler can't be exported. Maybe that should be the quick fix?

We also technically haven't cut a release for this commit, so I think we can still make some changes as a last-ditch effort if this doesn't sit well with you.

Loading

Copy link
Contributor Author

@tmdiep tmdiep Nov 16, 2020

Choose a reason for hiding this comment

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

The package docs looking strange is a good point. I've opened #3216.

Loading

Copy link
Contributor

@dsymonds dsymonds left a comment

If there's a desire to reuse code across the pubsub and pubsublite packages, it would probably be better for there to be an internal/pubsub package that both use, either using type aliases or merely having shallow types that forward to the internal/pubsub versions of types.

Loading

@tmdiep
Copy link
Contributor Author

@tmdiep tmdiep commented Nov 17, 2020

@dsymonds Thanks for the comments. No disagreement from me about the drawbacks of this API.

For context, we wanted to use pubsub.Message and pubsub.PublishResult not so much for implementation, but for their interface. There was lengthy discussion about this. We ultimately decided to emulate the pubsub.Topic.Publish and pubsub.Subscription.Receive interface for pubsublite in a compatibility subpackage, for customers who wanted to evaluate Pub/Sub Lite and run it with their existing Pub/Sub code with very minimal changes.

Loading

@dsymonds
Copy link
Contributor

@dsymonds dsymonds commented Nov 17, 2020

I wasn't privvy to any of the lengthy discussions about options (is there a doc anywhere?), but are you saying you want the pubsublite exported API to use the pubsub types like Message? And the main difference there would be to have the pubsublite construct Message values with a different ackHandler? (The other obvious option is to just copy the definition of Message to pubsublite and retain the same interface; it doesn't seem too hard to keep the two in sync manually.)

Keeping the NewMessage exported and exporting ackHandler seems like the wrong direction. It'd be better, sure, but you don't actually want anyone other than pubsub and pubsublite to invoke that, right? And the 99.9% of users of the pubsub package who would never invoke it would always be confused by its existence, even with ackHandler exported.

As an alternative: if there's an internal/pubsub package, it can have a little registration mechanism (and probably the ackHandler type definition) where the pubsub package can register a func that can be invoked by the pubsublite package to twiddle that field. Then internal/pubsub keeps the messy details out of sight, and the pubsub API is not affected.

Loading

@dsymonds
Copy link
Contributor

@dsymonds dsymonds commented Nov 17, 2020

Another alternative: move pubsub.Message into an internal/pubsub package, and write type Message = internalpubsub.Message in both pubsub and pubsublite. And then have the func NewMessage(AckHandler) in only internal/pubsub.

Loading

@tmdiep
Copy link
Contributor Author

@tmdiep tmdiep commented Nov 17, 2020

Keeping the NewMessage exported and exporting ackHandler seems like the wrong direction. It'd be better, sure, but you don't actually want anyone other than pubsub and pubsublite to invoke that, right? And the 99.9% of users of the pubsub package who would never invoke it would always be confused by its existence, even with ackHandler exported.

Some users wanted to verify whether ack/nack were called in their unit tests (#2920). AckHandler was intended to make this easier for them (as a side-effect). @hongalex who has more context on this aspect.

Loading

@dsymonds
Copy link
Contributor

@dsymonds dsymonds commented Nov 17, 2020

Thanks for forwarding me the extra discussion.

FWIW, if the long term plan is for pubsub and pubsublite to be independent packages, making the pubsub API messier to support pubsublite seems like the wrong move. A bit of copy and paste seems like a much better idea. If you match the types and signatures, the pubsublite package does indeed become almost an exact drop-in replacement just by replacing the import path. From your doc, Option 2 seems like a much more Go way to approach this. Consistency with Python/Java is not something that Go users typically care about. At the bottom of the doc it sounds like the Option 2 "extension" (which seems to be approximately the same idea) is what was decided, so I'm extra confused about this change, which seems to be preparing for Option 1.

FWIW, checking for Ack/Nack per #2920 is not a good reason either. As @tbpg pointed out there, there are better approaches (whether pstest, or defining ones own trivial interface) for testing.

Loading

@tmdiep
Copy link
Contributor Author

@tmdiep tmdiep commented Nov 17, 2020

If you match the types and signatures, the pubsublite package does indeed become almost an exact drop-in replacement just by replacing the import path. From your doc, Option 2 seems like a much more Go way to approach this.

About half of folks were original proponents of Option 2 (myself included) and had the same thoughts. We ultimately agreed with Option 2 extension, to move the Option 1 "pubsub shim" to a subpackage, so that we could reserve the pubsublite namespace for a future Lite-specific client that didn't have to be an exact drop-in replacement for pubsub, as it is a separate service with different semantics and features. I do feel that this was the best option.

Both Option 1 and Option 2 extension required some internal implementation details fields of pubsub.Message and pubsub.PublishResult to be accessible from outside the pubsub package. I've tried to make this as minimal as possible in this PR.

Another alternative: move pubsub.Message into an internal/pubsub package, and write type Message = internalpubsub.Message in both pubsub and pubsublite. And then have the func NewMessage(AckHandler) in only internal/pubsub.

I was almost going to say this sounds fine. But pubsub.Message.size and a couple of fields in psAckHandler are set after message creation. Which would require exposing even more fields. I'm not super familiar with the pubsub library, so not sure how much refactoring is required to set everything at message creation.

Loading

@dsymonds
Copy link
Contributor

@dsymonds dsymonds commented Nov 17, 2020

A few comments back I suggested moving the implementation parts that needed to be shared into an internal/pubsub package. You said "we wanted to use pubsub.Message and pubsub.PublishResult not so much for implementation, but for their interface", but now it sounds like you're saying that their implementation is what is what you want to share? If so, what's the drawback to moving those implementation details to an internal package that both pubsub and pubsublite (or the option 1 shim package) could use?

If you move it to internal/pubsub, you can export more fields (size, etc.) quite easily there, since there's no need for that package to be protective since nothing outside this repo can import it.

Loading

@tmdiep
Copy link
Contributor Author

@tmdiep tmdiep commented Nov 17, 2020

but now it sounds like you're saying that their implementation is what is what you want to share?

Sorry, I meant "internal fields" rather than "internal implementation details" (corrected). For example, we needed to access pubsub.PublishResult.set and override pubsub.Message.doneFunc. There was very little common implementation to share.

If so, what's the drawback to moving those implementation details to an internal package that both pubsub and pubsublite (or the option 1 shim package) could use?
If you move it to internal/pubsub, you can export more fields (size, etc.) quite easily there, since there's no need for that package to be protective since nothing outside this repo can import it.

@hongalex for preferences.

Hmm, I remember being told that users still use anything in internal/ directories (if so, being able to replace the ack handler of a message would not be ideal). But I probably misunderstood.

Loading

@hongalex
Copy link
Member

@hongalex hongalex commented Nov 17, 2020

Hmm, I remember being told that users still use anything in internal/ directories (if so, being able to replace the ack handler of a message would not be ideal). But I probably misunderstood.

I don't remember the context of this. I think we discussed that if pubsub and pubsublite are separate modules in the same directory structure, both could import internal from cloud.google.com/go/internal but not each other's internal directories.

As for preferences, I'm fine with anything that minimizes confusion with existing users and is consistent with how we maintain the pubsub/lite relationship in other languages. I'm in favor of exposing AckHandler because that's how Java does it (so there is precedent), and it makes testing easier for users.

Loading

@dsymonds
Copy link
Contributor

@dsymonds dsymonds commented Nov 17, 2020

There's already quite a bit of stuff under internal in this repo for this sort of purpose (sharing across packages, hiding away implementation details), but users aren't able to import them. https://golang.org/cmd/go/#hdr-Internal_Directories

Loading

@tmdiep
Copy link
Contributor Author

@tmdiep tmdiep commented Nov 18, 2020

Thanks both. To wrap up this discussion, we'll expose AckHandler - it at least provides some convenience for users' unit tests. The current contents of the internal directory look more generic than what we'd be putting in internal/pubsub. Ideally, the impact of the controversial pubsub shim is contained within the pubsub and pubsublite modules.

Loading

@tbpg
Copy link
Collaborator

@tbpg tbpg commented Nov 18, 2020

+1 to all of @dsymonds comments in this thread.

The current contents of the internal directory look more generic than what we'd be putting in internal/pubsub.

An internal directory is just for stuff that shouldn't be imported by other projects. This is the perfect use for it. And sticking all of the logic into a new Pub/Sub-specific internal package is a totally valid and normal use case.

Yes, the impact is contained to those two modules. But, we're talking about the exported API of the two modules. So, users outside of cloud.google.com/go will use them.

Loading

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

4 participants