feat(pubsub): configure TopicAdminConnection with Options #7336
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 #7336 +/- ##
=======================================
Coverage 93.66% 93.67%
=======================================
Files 1352 1352
Lines 117092 117122 +30
=======================================
+ Hits 109679 109711 +32
+ Misses 7413 7411 -2
Continue to review full report at Codecov.
|
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
There was a problem hiding this comment.
Reviewed 7 of 12 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dbolduc)
google/cloud/pubsub/topic_admin_connection.h, line 166 at r2 (raw file):
* * @warning This method exists solely for backwards compatibility. It prevents * existing code, which calls `MakeTopicAdminConnection({})` from breaking,
s/, which/that/
google/cloud/pubsub/internal/backwards_compatibility_type.h, line 23 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
I agree that it could be useful in the future for other libraries.
My thinking was just that we could start with it in
pubsub_internal, and only move it intog::c::internalif a use case in another library arises. I don't think that would constitute a breaking change, because nobody should be using the type.I don't feel strongly either way, though.
OK ... leaving it until that other use case might arise sounds reasonable.
google/cloud/pubsub/internal/backwards_compatibility_type.h, line 32 at r1 (raw file):
Previously, dbolduc (Darren Bolduc) wrote…
Do we like
NonConstructibleTypePleaseIgnore?
I think I would just go with just NonConstructible (Unconstructible?), and limit the description to something about cases where we want to disallow the instantiation of an object. (Might it sometimes not be about backwards compatibility?) That is, I'd suggest leaving mention of "initializer list" for the particular case that uses the unconstructible type that way (and drop mention of MakeTopicAdminConnection()). I'd also drop the PleaseIgnore.
|
google/cloud/pubsub/internal/backwards_compatibility_type.h, line 32 at r1 (raw file): Previously, devbww (Bradley White) wrote…
👍 to what @devbww said. |
|
/gcbrun |
1 similar comment
|
/gcbrun |
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
There was a problem hiding this comment.
Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @dbolduc)
google/cloud/pubsub/topic_admin_connection.h, line 165 at r3 (raw file):
* Creates a new `TopicAdminConnection` object to work with `TopicAdminClient`. * * @note This method exists solely for backwards compatibility. It prevents
FYI, I believe we try to avoid the term "method" for C++.
|
/gcbrun |
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Part of the work for #6306
This PR adds a method to configure
TopicAdminConnectionusingg::c::Options. Users who might have relied on callingMakeTopicAdminConnection({})will not be broken because of the introduction of a non-constructible backwards compatibility type. (Credit to @devbww for the idea and implementation).This PR also adds
Optionsalternatives forCreateDefaultPublisherStub()andCreateChannel(). The old methods are still used by the rest of theMake*Connectionmethods. Leaving them allows for incremental changes. I put in some TODO's to remind myself to remove the old methods when I am done, though.FYI: The same work for
TopicAdminConnectionwill also be done for:PublisherConnectionSubscriberConnectionSubscriptionAdminConnectionSchemaAdminConnection(I started with
TopicAdminConnection, because it didn't seem that involved)This change is