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(pubsublite): adding ability to create subscriptions at head #3790

Merged
merged 19 commits into from Mar 25, 2021

Conversation

@hannahrogers-google
Copy link
Contributor

@hannahrogers-google hannahrogers-google commented Mar 10, 2021

No description provided.

@google-cla google-cla bot added the cla: yes label Mar 10, 2021
@hannahrogers-google hannahrogers-google requested a review from tmdiep Mar 10, 2021
@codyoss codyoss changed the title feat: adding ability to create subscriptions at head feat(pubsublite): adding ability to create subscriptions at head Mar 10, 2021
Copy link
Contributor

@tmdiep tmdiep left a comment

Thanks Hannah!

pubsublite/admin.go Outdated Show resolved Hide resolved
pubsublite/admin.go Outdated Show resolved Hide resolved
pubsublite/admin.go Outdated Show resolved Hide resolved
pubsublite/admin.go Outdated Show resolved Hide resolved
pubsublite/admin.go Outdated Show resolved Hide resolved
Copy link

@avawes avawes left a comment

Hacked

tmdiep
tmdiep approved these changes Mar 12, 2021
pubsublite/admin.go Outdated Show resolved Hide resolved
pubsublite/admin.go Show resolved Hide resolved
@tmdiep
Copy link
Contributor

@tmdiep tmdiep commented Mar 16, 2021

Thanks Hannah! Still LGTM.

@hannahrogers-google
Copy link
Contributor Author

@hannahrogers-google hannahrogers-google commented Mar 19, 2021

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

Copy link
Collaborator

@tbpg tbpg left a comment

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

const (
// End refers to the location past all currently published messages. End
// skips the entire message backlog.
End BacklogLocation = iota + 1
Copy link
Collaborator

@tbpg tbpg Mar 19, 2021

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

Copy link
Contributor Author

@hannahrogers-google hannahrogers-google Mar 19, 2021

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.

Copy link
Contributor

@tmdiep tmdiep Mar 19, 2021

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

Copy link
Collaborator

@tbpg tbpg Mar 23, 2021

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?)

Copy link
Contributor Author

@hannahrogers-google hannahrogers-google Mar 23, 2021

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.

//
// By default, a new subscription will only receive messages published after
// the subscription was created. Use StartingOffset to override.
func (ac *AdminClient) CreateSubscription(ctx context.Context, config SubscriptionConfig, opts ...CreateSubscriptionOption) (*SubscriptionConfig, error) {
Copy link
Collaborator

@tbpg tbpg Mar 19, 2021

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?

Copy link
Contributor Author

@hannahrogers-google hannahrogers-google Mar 19, 2021

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

Copy link
Contributor

@tmdiep tmdiep Mar 19, 2021

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.

Copy link
Collaborator

@tbpg tbpg Mar 23, 2021

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

Copy link
Contributor Author

@hannahrogers-google hannahrogers-google Mar 23, 2021

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.

Copy link
Collaborator

@tbpg tbpg Mar 25, 2021

Got it. That makes sense. Thanks!

pubsublite/admin.go Show resolved Hide resolved
pubsublite/admin.go Show resolved Hide resolved
pubsublite/admin.go Show resolved Hide resolved
const (
// End refers to the location past all currently published messages. End
// skips the entire message backlog.
End BacklogLocation = iota + 1
Copy link
Collaborator

@tbpg tbpg Mar 23, 2021

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?)

//
// By default, a new subscription will only receive messages published after
// the subscription was created. Use StartingOffset to override.
func (ac *AdminClient) CreateSubscription(ctx context.Context, config SubscriptionConfig, opts ...CreateSubscriptionOption) (*SubscriptionConfig, error) {
Copy link
Collaborator

@tbpg tbpg Mar 23, 2021

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

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

@tbpg tbpg Mar 23, 2021

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
}

tbpg
tbpg approved these changes Mar 25, 2021
@hannahrogers-google hannahrogers-google merged commit bc083b6 into googleapis:master Mar 25, 2021
3 checks passed
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

6 participants