fix(pubsub): dont move PublisherOptions that we are still using #7270
Conversation
googlebot
added
the
cla: yes
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #7270 +/- ##
=======================================
Coverage 94.34% 94.34%
=======================================
Files 1317 1317
Lines 114595 114594 -1
=======================================
Hits 108112 108112
+ Misses 6483 6482 -1
Continue to review full report at Codecov.
|
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
| @@ -138,13 +138,13 @@ std::shared_ptr<pubsub::PublisherConnection> MakePublisherConnection( | |||
| if (options.message_ordering()) { | |||
| auto factory = [topic, options, sink, cq](std::string const& key) { | |||
| return BatchingPublisherConnection::Create( | |||
| topic, options, key, SequentialBatchSink::Create(std::move(sink)), | |||
| cq); | |||
| std::move(topic), std::move(options), key, | |||
There was a problem hiding this comment.
I do not think you want to std::move here. factory may be called multiple times, once for each ordering key that is discovered (dynamically). You want a copy, and note that they are captured by copy too.
| }; | ||
| return OrderingKeyPublisherConnection::Create(std::move(factory)); | ||
| } | ||
| return RejectsWithOrderingKey::Create(BatchingPublisherConnection::Create( | ||
| std::move(topic), std::move(options), {}, sink, std::move(cq))); | ||
| topic, options, {}, std::move(sink), std::move(cq))); |
There was a problem hiding this comment.
This is called at most once, and therefore a std::move(topic) was appropriate.
| }; | ||
| return OrderingKeyPublisherConnection::Create(std::move(factory)); | ||
| } | ||
| return RejectsWithOrderingKey::Create(BatchingPublisherConnection::Create( | ||
| std::move(topic), std::move(options), {}, sink, std::move(cq))); | ||
| topic, options, {}, std::move(sink), std::move(cq))); | ||
| }; | ||
| auto connection = make_connection(); |
There was a problem hiding this comment.
I am beginning to think this is too clever for its own good.
Maybe we should do this (starting around line 133):
auto cp = background->cq();
auto sink = DefaultBatchSink::Create(stub, cq, std::move(retry_policy), std::move(backoff_policy));
auto connection = RejectsWithOrderingKey::Create(BatchingPublisherConnection::Create(topic, options, {}, sink, cq);
if (options.message_ordering()) {
auto factory = [topic, options, sink, cq](std::string const& key) {
return BatchingPublisherConnection::Create(topic, options, key, SequentialBatchSink(std::move(sink), std::move(cq)));
};
connection = OrderKeyPublisherConnection(std::move(factory));
}It makes a few more copies than it should, but it is probably more readable. Alternatively, moving the lambda to a function may make it more obvious. Your call.
There was a problem hiding this comment.
I think the code is fine. I like that BatchingPublisherConnection::Create() is only called once
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
we
std::move(options)in themake_connectionlambda which captures by reference. Then we keep usingoptions...I am not sure how to test for this change.
This change is