feat(pubsub): introduce PublisherOptionList (g::c::Options) #7308
Conversation
google-cla
bot
added
the
cla: yes
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #7308 +/- ##
==========================================
+ Coverage 93.67% 93.69% +0.02%
==========================================
Files 1344 1347 +3
Lines 116443 116537 +94
==========================================
+ Hits 109074 109189 +115
+ Misses 7369 7348 -21
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Reviewable status: 0 of 12 files reviewed, 6 unresolved discussions (waiting on @coryan and @devbww)
google/cloud/pubsub/options.h, line 44 at r1 (raw file):
Previously, devbww (Bradley White) wrote…
Unused?
Done.
google/cloud/pubsub/options.h, line 54 at r1 (raw file):
Previously, devbww (Bradley White) wrote…
Nit: Some notes have initial caps, and some don't.
Fixed in this PR. And: #7310
google/cloud/pubsub/publisher.cc, line 24 at r1 (raw file):
Previously, devbww (Bradley White) wrote…
I assume that at some point in the future these will be moved somewhere?
I am not sure I follow what you are asking.
I assume that there is currently a PublisherOptions param for backwards compatibility, which is now ignored because the PublisherOptions have already been used to configure the PublisherConnection.
I think at some point in the future PublisherOptions will be deprecated, and we will just make a new constructor that only accepts a std::shared_ptr<PublisherConnection> connection.
did that answer your question?
google/cloud/pubsub/publisher_options.h, line 269 at r1 (raw file):
Previously, devbww (Bradley White) wrote…
Is return by rref needed, or even good?
It is definitely not needed. Based on the types of the options, I don't think it adds anything. Fixed.
google/cloud/pubsub/internal/defaults.h, line 26 at r1 (raw file):
Previously, devbww (Bradley White) wrote…
If the only place you envisage defaulting the argument is in the test, then I'd remove the default.
Fixed.
google/cloud/pubsub/internal/defaults.cc, line 37 at r1 (raw file):
Previously, devbww (Bradley White) wrote…
#include <limits>
Done.
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
There was a problem hiding this comment.
Reviewed 6 of 12 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @coryan and @dbolduc)
google/cloud/pubsub/publisher.cc, line 24 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
I am not sure I follow what you are asking.
I assume that there is currently a
PublisherOptionsparam for backwards compatibility, which is now ignored because thePublisherOptionshave already been used to configure thePublisherConnection.I think at some point in the future
PublisherOptionswill be deprecated, and we will just make a new constructor that only accepts astd::shared_ptr<PublisherConnection> connection.did that answer your question?
If the parameter is unused, can we just make it a const& and avoid a copy and a NOLINT?
google/cloud/pubsub/publisher_options_test.cc, line 113 at r2 (raw file):
.enable_message_ordering(); auto opts = pubsub_internal::MakeOptions(b);
It seems like it is still a benefit to move these PublisherOptions into the argument if the local variable is otherwise dead.
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
There was a problem hiding this comment.
Reviewable status: 10 of 13 files reviewed, 2 unresolved discussions (waiting on @coryan and @devbww)
google/cloud/pubsub/publisher.cc, line 24 at r1 (raw file):
Previously, devbww (Bradley White) wrote…
If the parameter is unused, can we just make it a
const&and avoid a copy and aNOLINT?
Done.
google/cloud/pubsub/publisher_options_test.cc, line 113 at r2 (raw file):
Previously, devbww (Bradley White) wrote…
It seems like it is still a benefit to move these
PublisherOptionsinto the argument if the local variable is otherwise dead.
Done.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @coryan)
(Continuation of #7273) I think I was supposed to reopen the closed PR then push to it, instead of force-pushing when it was closed.
This change is