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): Types for resource paths and topic/subscription configs #3026

Merged
merged 5 commits into from Oct 15, 2020

Conversation

@tmdiep
Copy link
Contributor

@tmdiep tmdiep commented Oct 15, 2020

{Topic|Subscription}ConfigToUpdate are really just a subset of {Topic|Subscription}Config, but I adopted the *ToUpdate pattern of some of the other client libraries and for future flexibility and to provide clearer documentation.

The config fields were flattened for simplicity.

Type and field names were chosen to match Cloud Pub/Sub, where there is overlap.

See http://go/pubsublite-go-clientlib-interface.

@tmdiep tmdiep requested a review from googleapis/yoshi-go-admins as a code owner Oct 15, 2020
@google-cla google-cla bot added the cla: yes label Oct 15, 2020
@tmdiep tmdiep requested a review from hongalex Oct 15, 2020
@tmdiep
Copy link
Contributor Author

@tmdiep tmdiep commented Oct 15, 2020

I also wanted to add manuelmenzella-google as reviewer, but the account didn't appear in the list.

Also, should we set owners for pubsublite? For the smaller future clean ups, we probably don't need yoshi-go-admins.

Copy link
Member

@hongalex hongalex left a comment

Left some minor nits and comments, but otherwise LGTM

pubsublite/types.go Outdated Show resolved Hide resolved
pubsublite/types.go Show resolved Hide resolved
pubsublite/types.go Outdated Show resolved Hide resolved
pubsublite/types.go Outdated Show resolved Hide resolved
pubsublite/types.go Show resolved Hide resolved
@tmdiep tmdiep removed the request for review from googleapis/yoshi-go-admins Oct 15, 2020
tmdiep added 2 commits Oct 15, 2020
@tmdiep
Copy link
Contributor Author

@tmdiep tmdiep commented Oct 15, 2020

Thanks Alex, comments addressed.

@gcf-merge-on-green gcf-merge-on-green bot merged commit 6f7fa86 into googleapis:master Oct 15, 2020
3 checks passed
3 checks passed
@google-cla
cla/google All necessary CLAs are signed
@conventional-commit-lint-gcf
conventionalcommits.org
Details
@kokoro-team
kokoro Kokoro CI build successful
Details
@tmdiep tmdiep deleted the tmdiep:config branch Oct 15, 2020
Copy link
Member

@codyoss codyoss left a comment

Sorry for not getting to this sooner. I thought I would just leave some things to consider if there is still flexibility in the API.

pubsublite/config.go Show resolved Hide resolved
pubsublite/config.go Show resolved Hide resolved
pubsublite/types.go Show resolved Hide resolved
pubsublite/types.go Show resolved Hide resolved
// about how topics are configured.
type TopicConfig struct {
// The full path of a topic.
Name TopicPath

This comment has been minimized.

@codyoss

codyoss Oct 19, 2020
Member

I wonder if it worth inlining all of these structs that are facades of resource names would be useful?

This comment has been minimized.

@tmdiep

tmdiep Oct 19, 2020
Author Contributor

Hmm... Unlike most other services, Pub/Sub Lite resources are zonal, hence topics are globally uniquely identified by {project, zone, id} and it's common to have to copy all components across to different objects, function args, etc. I think keeping the fields together as a full path is convenient and less error prone.


// TopicPath stores the full path of a Google Pub/Sub Lite topic.
// See https://cloud.google.com/pubsub/lite/docs/topics for more information.
type TopicPath struct {

This comment has been minimized.

@codyoss

codyoss Oct 19, 2020
Member

I wonder if this could struct could be refactored out. Maybe the client could take a zone and project in the constructor. Then topic methods could just request an ID. Then there would be no need for TopicPath, SubscriptionPath, and LocationPath.

This comment has been minimized.

@tmdiep

tmdiep Oct 19, 2020
Author Contributor

We can have cross-project subscriptions, where the topic and subscription resources are not necessarily owned by the same project. So having a default project set for the client could cause some confusion for, e.g. Create/Update Subscription.

I also saw that pubsub.Client had evolved to support variants Topic/TopicInProject, Subscription/SubscriptionInProject, so wanted to avoid this, as it seems multi-project admin does occur.

Similar remarks for zone, as the admin server is regional. Users can use the same client to manage all resources within the region.

For these reasons, I thought it would be best to give the client more flexibility with projects and zones.

@tmdiep
Copy link
Contributor Author

@tmdiep tmdiep commented Oct 19, 2020

Thanks Cody, I'll address most of your comments in #3036.

tmdiep added a commit to tmdiep/google-cloud-go that referenced this pull request Oct 20, 2020
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

3 participants