feat(pubsublite): adding ability to create subscriptions at head

[hannahrogers-google]

No description provided.

[tmdiep]

Thanks Hannah!

[avawes]

Hacked

[tmdiep]

Thanks Hannah! Still LGTM.

[hannahrogers-google]

Friendly ping @tbpg or @codyoss, can you PTAL at the API surface changes?

[tbpg]

Sorry for the slow review! I'm not familiar with the feature in and of itself, but left a few comments.

[tbpg]

Will it be clear from the name that this is a BacklogLocation? I'm thinking about reading code that uses this: pubsublite.End isn't totally clear what End is.

(I'm not familiar with the feature, so totally fine if you say, yes, it's clear in context.)

[hannahrogers-google]

Yes, this should be clear to users based on context. When calling the create subscription method, users will have to specify StartingOffset(Beginning/End) - The StartingOffset should indicate that the option between beginning/end is a location in backlog.

[tmdiep]

If this is any clearer, perhaps we can consider StartingAt(BeginningOfBacklog|EndOfBacklog).

[tbpg]

I'm good with any of the above. One other idea would be basing it on "oldest" and "newest" (is the end new or old?)

[hannahrogers-google]

End represents the newest message, Beginning represents the oldest retained. My preference would be to keep this as End/Beginning, as this is what we use for gCloud, the UI, and other client libraries.

[tbpg]

This is, unfortunately, a breaking change. Users may have defined an interface with the CreateSubscription(context.Context, pubsublite.SubscriptionConfig) method. If we change this signature, AdminClient will no longer fulfill that interface.

Is that OK?

Alternatively, could we add it to the SubscriptionConfig?

[hannahrogers-google]

This should be fine, I don't believe this library has been released yet. Right @tmdiep?

[tmdiep]

The library is released as Beta, but since that was only recent, the likelihood of a user creating an interface for AdminClient is very low. This low risk is fine to me.

[tbpg]

For my own understanding, why don't we want to put it into the config?

(The two common patterns for functions like this are config arguments and options -- I'm surprised to see both.)

[hannahrogers-google]

All of the fields in the SubscriptionConfig map directly to a field in the Subscription resource proto - these fields are defining features of the subscription itself. On the other hand, the StartingOffset option only makes sense on subscription creation - it is not part of the subscription resource and does not map to any field in the subscription proto (it instead maps to a field in the create subscription request).

The SubscriptionConfig is used elsewhere in the library (ex. it is returned from the UpdateSubscription request). For this reason, I think that it is misleading to have the StartingOffset in the subscription config when it only makes sense on subscription creation.

[tbpg]

Got it. That makes sense. Thanks!

[tbpg]

I'm good with any of the above. One other idea would be basing it on "oldest" and "newest" (is the end new or old?)

[tbpg]

For my own understanding, why don't we want to put it into the config?

(The two common patterns for functions like this are config arguments and options -- I'm surprised to see both.)

[tbpg]

Can avoid the type assertion with Cody's idea above. But, for future type assertions, you can do this:

Suggested change

	if _, ok := opt.(startingOffset); ok {
	skipBacklog = opt.(startingOffset).backlogLocation != Beginning
	}
	if s, ok := opt.(startingOffset); ok {
	skipBacklog = s.backlogLocation != Beginning
	}