Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(pubsublite): adding ability to create subscriptions at head #3790
Changes from 14 commits
e1fdb8d
1b78089
98d5b84
b56227a
569e613
19f2052
20e4c8b
0a73b53
f417124
cf4eba9
909dd3d
52c3332
23f4a90
99cd943
c09e7e6
16b10fc
18f7f2e
e2d173f
066cef9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 whatEnd
is.(I'm not familiar with the feature, so totally fine if you say, yes, it's clear in context.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is any clearer, perhaps we can consider StartingAt(BeginningOfBacklog|EndOfBacklog).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fine, I don't believe this library has been released yet. Right @tmdiep?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. That makes sense. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can avoid the type assertion with Cody's idea above. But, for future type assertions, you can do this: