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

fix(pubsublite)!: add separate publisher and subscriber client constructors with settings (api review) #3528

Merged
merged 10 commits into from Jan 27, 2021

Conversation

tmdiep
Copy link
Contributor

@tmdiep tmdiep commented Jan 12, 2021

We expect most users to just use default publish/receive settings. The NewPublisherClient and NewSubscriberClient constructors will omit the settings arg (using default settings). The additional *WithSettings constructors will allow users to specify settings. This is consistent with NewClient/NewClientWithConfig in the bigtable and spanner libraries.

AI from API review.

Equivalent to default settings. Updated samples.
@tmdiep tmdiep requested a review from hongalex January 12, 2021 02:56
@tmdiep tmdiep requested a review from a team as a code owner January 12, 2021 02:56
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jan 12, 2021
@tmdiep tmdiep marked this pull request as draft January 12, 2021 03:46
@tmdiep
Copy link
Contributor Author

tmdiep commented Jan 12, 2021

Converted this PR to draft as I'm considering adding NewPublisherClient() and NewPublisherClientWithSettings() for consistency with bigtable and spanner. Pending objections via email.

@tmdiep tmdiep changed the title feat(pubsublite)!: allow nil settings for new publisher and subscriber feat(pubsublite)!: add separate publisher and subscriber client constructors with settings Jan 12, 2021
@tmdiep tmdiep marked this pull request as ready for review January 12, 2021 23:22
@tmdiep tmdiep requested a review from codyoss January 12, 2021 23:22
@tmdiep
Copy link
Contributor Author

tmdiep commented Jan 12, 2021

WANT_LGTM=all

After seeing NewClientWithConfig in the spanner and bigtable libraries, I thought it would be better to be consistent. And possibly less confusing without the nil arg.

I considered renaming *Settings to *Config, but these are meant to mirror pubsub.PublishSettings and pubsub.ReceiveSettings.

pubsublite/ps/publisher.go Outdated Show resolved Hide resolved
@tmdiep tmdiep changed the title feat(pubsublite)!: add separate publisher and subscriber client constructors with settings fix(pubsublite)!: add separate publisher and subscriber client constructors with settings (api review) Jan 13, 2021
@tmdiep tmdiep added the api: pubsublite Issues related to the Pub/Sub Lite API. label Jan 15, 2021
@tmdiep tmdiep added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 27, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 27, 2021
@tmdiep tmdiep merged commit 98637e0 into googleapis:master Jan 27, 2021
@tmdiep tmdiep deleted the settings_pointer branch January 27, 2021 21:52
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.

None yet

5 participants