-
Notifications
You must be signed in to change notification settings - Fork 104
feat(pubsub): add simple Publisher builder #4343
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
base: main
Are you sure you want to change the base?
feat(pubsub): add simple Publisher builder #4343
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4343 +/- ##
==========================================
- Coverage 94.87% 94.65% -0.22%
==========================================
Files 188 188
Lines 7237 7276 +39
==========================================
+ Hits 6866 6887 +21
- Misses 371 389 +18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7efb993 to
b2cd951
Compare
dbolduc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you fix the examples to use the right client?
And can you also do a pass on the coverage for some of the new options? #4343 (comment)
The nits around wording are not a blocker for me approving
| } | ||
|
|
||
| impl Publisher { | ||
| /// Creates `Publisher`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// Returns a builder for [Publisher]
| Ok(publisher) | ||
| } | ||
|
|
||
| // Configure the batching options. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove?
|
|
||
| // Configure the batching options. | ||
|
|
||
| /// Sets the maximum number of messages to be batched together for a single `Publish` call. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I know this PR is copying existing documentation, but we typically start docs with a short summary sentence like:
/// Set the maximum number of messages in a batch.
///
/// Blah blah blah more details.
https://doc.rust-lang.org/rustdoc/how-to-write-documentation.html#documenting-components
| self | ||
| } | ||
|
|
||
| /// Sets the byte threshold for batching in a single `Publish` call. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: same, Set the maximum size for the messages in a batch.
(I am not coming up with these btw, I am copying the C++ docs: https://github.com/googleapis/google-cloud-cpp/blob/9694430272e74ce3dc9702e180b4bc7f7a61b708/google/cloud/pubsub/options.h#L110)
| self | ||
| } | ||
|
|
||
| /// Sets the maximum amount of time the publisher will wait before sending a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same: "The maximum hold time for the messages." ?
| // Configure the client. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
| /// # use gax::client_builder::examples; | ||
| /// # use gax::client_builder::Result; | ||
| /// # tokio_test::block_on(async { | ||
| /// use examples::Client; // Placeholder for examples |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to use Publisher in the Publisher documentation, not a made up examples::Client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below.
| /// This is the main entry point for the publisher API. A single `BasePublisher` | ||
| /// can be used to create multiple `Publisher` clients for different topics. | ||
| /// A single `BasePublisher` can be used to create multiple `Publisher` | ||
| /// clients for different topics. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: reflow?
Add a new builder that can configure both the client settings and the batching settings for the Publisher and updated the examples to use the new builder.
The doc comments and examples are copied from the ClientBuilder and the PublisherPartialBuilder. The only client configuration that is not included is the polling configuration since this isn't relevant.
For #4282