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): Pub/Sub Lite admin client #3036

Merged
merged 16 commits into from
Oct 26, 2020

Conversation

tmdiep
Copy link
Contributor

@tmdiep tmdiep commented Oct 16, 2020

Implements pubsublite.Client, which wraps the Pub/Sub Lite Admin Service.
Includes integration tests for admin operations.

@tmdiep tmdiep requested a review from a team as a code owner October 16, 2020 02:27
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 16, 2020
@tmdiep tmdiep requested a review from hongalex October 16, 2020 02:28
@tmdiep
Copy link
Contributor Author

tmdiep commented Oct 16, 2020

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 product-auto-label bot added the api: pubsublite Issues related to the Pub/Sub Lite API. label Oct 16, 2020
pubsublite/integration_test.go Outdated Show resolved Hide resolved
pubsublite/integration_test.go Outdated Show resolved Hide resolved
pubsublite/pubsublite.go Outdated Show resolved Hide resolved
pubsublite/integration_test.go Show resolved Hide resolved
pubsublite/integration_test.go Outdated Show resolved Hide resolved
pubsublite/pubsublite.go Outdated Show resolved Hide resolved
pubsublite/pubsublite.go Outdated Show resolved Hide resolved
pubsublite/pubsublite.go Outdated Show resolved Hide resolved
pubsublite/pubsublite.go Outdated Show resolved Hide resolved
pubsublite/pubsublite.go Outdated Show resolved Hide resolved
pubsublite/pubsublite.go Outdated Show resolved Hide resolved
@tmdiep tmdiep added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 20, 2020
@tmdiep
Copy link
Contributor Author

tmdiep commented Oct 20, 2020

Thanks both, all comments addressed.
But I'd like to get Manu to review as well, once it's possible!

@tmdiep tmdiep removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 21, 2020
@tmdiep
Copy link
Contributor Author

tmdiep commented Oct 21, 2020

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.

@tmdiep tmdiep requested review from codyoss and hongalex October 21, 2020 21:45
Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to block on these as more iteration can happen later. Just some thoughts.

pubsublite/config.go Show resolved Hide resolved
}

// DeleteTopic deletes a topic.
func (ac *AdminClient) DeleteTopic(ctx context.Context, topic TopicPath) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@tmdiep tmdiep Oct 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 tmdiep added the automerge Merge the pull request once unit tests and other checks pass. label Oct 22, 2020
@gcf-merge-on-green
Copy link
Contributor

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

@tmdiep tmdiep removed the request for review from manuelmenzella-google October 23, 2020 01:01
@gcf-merge-on-green
Copy link
Contributor

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
@gcf-merge-on-green
Copy link
Contributor

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 gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Oct 23, 2020
@tmdiep tmdiep merged commit 749473e into googleapis:master Oct 26, 2020
@tmdiep tmdiep deleted the admin_client branch October 26, 2020 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsublite Issues related to the Pub/Sub Lite API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants