feat(pubsub): Allow Message and PublishResult to be used outside the package

[tmdiep]

• 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.

[hongalex]

LGTM, thanks!

[hongalex]

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 added benefit of making the pubsub package easier to test (originally proposed in #2920)

[dsymonds]

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

[hongalex]

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.

[tmdiep]

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

[dsymonds]

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.

[tmdiep]

@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.

[dsymonds]

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.

[dsymonds]

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.

[tmdiep]

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.

[dsymonds]

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.

[tmdiep]

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.

[dsymonds]

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.

[tmdiep]

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.

[hongalex]

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.

[dsymonds]

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

[tmdiep]

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.

[tbpg]

+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.