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 Jan 12, 2021
@tmdiep tmdiep requested a review from as a code owner Jan 12, 2021
@google-cla google-cla bot added the cla: yes label Jan 12, 2021
@tmdiep tmdiep marked this pull request as draft Jan 12, 2021
@tmdiep
Copy link
Contributor Author

@tmdiep 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 Jan 12, 2021
@tmdiep tmdiep requested a review from codyoss Jan 12, 2021
@tmdiep
Copy link
Contributor Author

@tmdiep 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 force-pushed the settings_pointer branch from abaeb0c to f460cb8 Jan 22, 2021
tbpg
tbpg approved these changes Jan 27, 2021
@tmdiep tmdiep merged commit 98637e0 into googleapis:master Jan 27, 2021
3 checks passed
@tmdiep tmdiep deleted the settings_pointer branch Jan 27, 2021
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

5 participants