-
Notifications
You must be signed in to change notification settings - Fork 14
feat: add SessionPoolOptions with labels to MakeConnection() #1109
Conversation
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.
|
5f13550
to
edcedd7
Compare
google/cloud/spanner/client.h, line 549 at r2 (raw file):
clang-tidy thinks this line should wrap |
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.
after fixing the clang-tidy thing (and assuming the other build failure was bogus)
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. |
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.
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,
ConnectionOptions
isconst &
(and probablyDatabase
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 themove
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.
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.
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.
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.
Moves
SessionPoolOptions
out of theinternal
namespace,and adds a
std::map<std::string, std::string> labels
field.Plumbs those labels through to the
BatchCreateSessions()
call.
Fixes #421.
This change is