feat: add SessionPoolOptions with labels to MakeConnection()#1109
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #1109 +/- ##
=========================================
+ Coverage 87.95% 88% +0.05%
=========================================
Files 163 164 +1
Lines 12299 12315 +16
=========================================
+ Hits 10817 10838 +21
+ Misses 1482 1477 -5
Continue to review full report at Codecov.
|
devbww
force-pushed
the
labels
branch
2 times, most recently
from
5f13550 to
edcedd7
Compare
|
google/cloud/spanner/client.h, line 549 at r2 (raw file):
clang-tidy thinks this line should wrap |
mr-salty
left a comment
mr-salty
left a comment
There was a problem hiding this comment.
Reviewed 10 of 12 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @devbww)
google/cloud/spanner/client.h, line 558 at r2 (raw file):
Database const& db, ConnectionOptions const& connection_options, SessionPoolOptions session_pool_options,
FWIW, ConnectionOptions is const & (and probably Database as well) because if it's not, we get a clang-tidy warning about "moving a trivial type" or similar, and then if you remove the move it complains about it being copied.
So, I'm guessing SessionPoolOptions doesn't have that issue due to the map, but it seems really odd that these are different, and that will show up at the call site ...(db, conn_options, std::move(pool_options));
IDK what we can/should do about it, although Greg mentioned tweaking clang-tidy warnings about this in the past.
google/cloud/spanner/internal/session_pool.h, line 27 at r2 (raw file):
#include "google/cloud/status_or.h" #include <google/spanner/v1/spanner.pb.h> #include <chrono>
this can be deleted
Moves `SessionPoolOptions` out of the `internal` namespace, and adds a `std::map<std::string, std::string> labels` field. Plumbs those labels through to the `BatchCreateSessions()` call. Fixes googleapis#421.
Fixed in #1122. |
Done. |
Ack, although for now it is what it is. |
devbww
left a comment
devbww
left a comment
There was a problem hiding this comment.
Reviewable status: 12 of 13 files reviewed, 3 unresolved discussions (waiting on @mr-salty)
google/cloud/spanner/client.h, line 549 at r2 (raw file):
Previously, mr-salty (Todd Derr) wrote…
clang-tidy thinks this line should wrap
Done.
google/cloud/spanner/client.h, line 558 at r2 (raw file):
Previously, mr-salty (Todd Derr) wrote…
Database const& db, ConnectionOptions const& connection_options, SessionPoolOptions session_pool_options,FWIW,
ConnectionOptionsisconst &(and probablyDatabaseas well) because if it's not, we get a clang-tidy warning about "moving a trivial type" or similar, and then if you remove themoveit complains about it being copied.So, I'm guessing
SessionPoolOptionsdoesn't have that issue due to the map, but it seems really odd that these are different, and that will show up at the call site...(db, conn_options, std::move(pool_options));IDK what we can/should do about it, although Greg mentioned tweaking clang-tidy warnings about this in the past.
Done.
google/cloud/spanner/internal/session_pool.h, line 27 at r2 (raw file):
Previously, mr-salty (Todd Derr) wrote…
this can be deleted
Done.
devbww
left a comment
devbww
left a comment
There was a problem hiding this comment.
Dismissed @mr-salty from 3 discussions.
Reviewable status: 12 of 13 files reviewed, all discussions resolved (waiting on @mr-salty)
…pis/google-cloud-cpp-spanner#1109) Moves `SessionPoolOptions` out of the `internal` namespace, and adds a `std::map<std::string, std::string> labels` field. Plumbs those labels through to the `BatchCreateSessions()` call. Fixes googleapis/google-cloud-cpp-spanner#421.
3 participants
Moves
SessionPoolOptionsout of theinternalnamespace,and adds a
std::map<std::string, std::string> labelsfield.Plumbs those labels through to the
BatchCreateSessions()call.
Fixes #421.
This change is