feat(pubsublite): Pub/Sub Lite admin client #3036
Conversation
google-cla
bot
added
the
cla: yes
|
Unit tests will be in a future PR, as they would make this one too large and I figured integration tests are more worthwhile. |
product-auto-label
bot
added
the
api: pubsublite
tmdiep
added
the
do not merge
|
Thanks both, all comments addressed. |
tmdiep
removed
the
do not merge
|
Manu will be OOO the next couple of days, so I'll get this committed to unblock dependent PRs. This is ready for approval and merge now. |
| } | ||
|
|
||
| // DeleteTopic deletes a topic. | ||
| func (ac *AdminClient) DeleteTopic(ctx context.Context, topic TopicPath) error { |
There was a problem hiding this comment.
Here, and with other methods, XXXPath does not seems like a meaningful name. It makes sense if you know that it is used to build a resource name, but if you don't know that the name is confusing in my opinion. I wonder if there is a way to refactor this so that all you need to pass to the method is a context and TopicID.
There was a problem hiding this comment.
The topic is uniquely identified by its {project, zone, topic id}. I didn't store the project in the client because 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.e. is the default project used for the topic or the subscription?).
I also saw that pubsub.Client had evolved to support multiple 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; minimizing the number of methods.
There was a problem hiding this comment.
I forgot to mention: we have planned regional replication for topics (b/171412217), so the admin client could end up dealing with multiple zones within the same region, so storing a default zone could cause confusion and/or proliferation of method variants.
tmdiep
added
the
automerge
|
Your PR has attempted to merge for 3 hours. Please check that all required checks have passed, you have an automerge label, and that all your reviewers have approved the PR |
|
Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, or one of your required reviews was not approved. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot. |
1 similar comment
|
Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, or one of your required reviews was not approved. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot. |
gcf-merge-on-green
bot
removed
the
automerge
Implements pubsublite.Client, which wraps the Pub/Sub Lite Admin Service.
Includes integration tests for admin operations.