feat: add some plumbing to enable multiple channels #1050

mr-salty · 2019-11-13T21:45:09Z

Note that Client might create multiple channels but the SessionPool
only uses the first one right now. I'll add proper support for multiple
channels in a subsequent change but wanted to get some of the more
mechanical parts out of the way.

This change is

mr-salty · 2019-11-13T21:48:15Z

google/cloud/spanner/client.cc, line 226 at r1 (raw file):

  stubs.reserve(num_channels);
  for (int i = 0; i < num_channels; ++i) {
    stubs.push_back(internal::CreateDefaultSpannerStub(options));

@coryan to resolve our discussion from yesterday, should I modify the channel_pool_domain for each one of these channels? append the value of i to each of them or similar? kind of a pain that options isn't mutable.

coryan

Reviewed 10 of 10 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mr-salty and @scotthart)

google/cloud/spanner/client.cc, line 226 at r1 (raw file):

Previously, mr-salty (Todd Derr) wrote…

@coryan to resolve our discussion from yesterday, should I modify the channel_pool_domain for each one of these channels? append the value of i to each of them or similar? kind of a pain that options isn't mutable.

I think we need to let the user pick channel_pool_domain (in case they want different pools for background ops vs. serving ops for example) and use something else for the channel id, maybe grpc.channel_id (which I took from here):

https://grpc.github.io/grpc/core/group__grpc__arg__keys.html

google/cloud/spanner/connection_options.cc, line 38 at r1 (raw file):

    : credentials_(std::move(credentials)),
      endpoint_("spanner.googleapis.com"),
      num_channels_(1),

FYI: This is unlikely to be a good default in the future. Maybe something like "number of cores" or "N x number_of_cores".

google/cloud/spanner/connection_options_test.cc, line 53 at r1 (raw file):

TEST(ConnectionOptionsTest, NumChannels) {
  ConnectionOptions options(grpc::InsecureChannelCredentials());
  EXPECT_EQ(1, options.num_channels());

Maybe just check for non-zero here.

codecov · 2019-11-13T22:07:00Z

Codecov Report

❗ No coverage uploaded for pull request base (master@13cc600). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #1050   +/-   ##
=========================================
  Coverage          ?   91.58%           
=========================================
  Files             ?      163           
  Lines             ?    11736           
  Branches          ?        0           
=========================================
  Hits              ?    10748           
  Misses            ?      988           
  Partials          ?        0

Impacted Files	Coverage Δ
google/cloud/spanner/internal/spanner_stub.h	`100% <ø> (ø)`
google/cloud/spanner/connection_options.cc	`91.17% <ø> (ø)`
google/cloud/spanner/internal/session_pool.cc	`93.26% <100%> (ø)`
.../spanner/benchmarks/multiple_rows_cpu_benchmark.cc	`77.64% <100%> (ø)`
google/cloud/spanner/internal/spanner_stub.cc	`70.32% <100%> (ø)`
google/cloud/spanner/internal/session_pool_test.cc	`99.16% <100%> (ø)`
google/cloud/spanner/internal/spanner_stub_test.cc	`100% <100%> (ø)`
google/cloud/spanner/connection_options_test.cc	`100% <100%> (ø)`
google/cloud/spanner/client.cc	`82.52% <100%> (ø)`
google/cloud/spanner/internal/connection_impl.cc	`96.31% <100%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13cc600...0c16ec5. Read the comment docs.

Note that `Client` might create multiple channels but the `SessionPool` only uses the first one right now. I'll add proper support for multiple channels in a subsequent change but wanted to get some of the more mechanical parts out of the way.

mr-salty

Reviewable status: 6 of 14 files reviewed, 2 unresolved discussions (waiting on @coryan and @scotthart)

google/cloud/spanner/client.cc, line 226 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

I think we need to let the user pick channel_pool_domain (in case they want different pools for background ops vs. serving ops for example) and use something else for the channel id, maybe grpc.channel_id (which I took from here):

https://grpc.github.io/grpc/core/group__grpc__arg__keys.html

Done, although I'm not entirely sure if we need to pass the channel_id in or if I could just i.e. have an atomic inside CreateDefaultSpannerStub that I increment every time.

I didn't want to put the channel_id in ConnectionOptions though, since the user shouldn't be setting it.

google/cloud/spanner/connection_options.cc, line 38 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

FYI: This is unlikely to be a good default in the future. Maybe something like "number of cores" or "N x number_of_cores".

I left it at 1 with a comment in the header, since we really only support 1 right now. For java the default is 4, so I'm inclined to do that rather than making it dynamic based on number of CPUs.

google/cloud/spanner/connection_options_test.cc, line 53 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

Maybe just check for non-zero here.

Done (and I ensured what we set the 2nd time is not the default)

google/cloud/spanner/benchmarks/multiple_rows_cpu_benchmark.cc, line 315 at r2 (raw file):

      auto options = cs::ConnectionOptions().set_channel_pool_domain(
          "task:" + std::to_string(i));

@coryan now that I'm passing i as channel_id below can we remove this line? I didn't want to mess up your benchmarks without asking.

coryan

Reviewed 8 of 8 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mr-salty and @scotthart)

google/cloud/spanner/client.cc, line 226 at r1 (raw file):

I didn't want to put the channel_id in ConnectionOptions though, since the user shouldn't be setting it.

I agree.

google/cloud/spanner/benchmarks/multiple_rows_cpu_benchmark.cc, line 315 at r2 (raw file):

Previously, mr-salty (Todd Derr) wrote…

      auto options = cs::ConnectionOptions().set_channel_pool_domain(
          "task:" + std::to_string(i));
@coryan now that I'm passing i as channel_id below can we remove this line? I didn't want to mess up your benchmarks without asking.

Not quite. The intention here is to create different cs::Client objects backed by different connections. Ideally, once all your changes are in, the performance with 1 client or with N clients will be the same.

google/cloud/spanner/internal/spanner_stub.h, line 107 at r2 (raw file):

/**
 * Creates a SpannerStub configured with @p options.
 * @p channel_id should be unique among all stubs in the same Connection pool,

nit: this does not read as part of the "brief" description (which is what the first line in a Doxygen comment becomes). Maybe separate the brief description from the longer description of what channel_id is for?

mr-salty

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @coryan and @scotthart)

google/cloud/spanner/benchmarks/multiple_rows_cpu_benchmark.cc, line 315 at r2 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

Not quite. The intention here is to create different cs::Client objects backed by different connections. Ideally, once all your changes are in, the performance with 1 client or with N clients will be the same.

ok I just made the channel_id 0

google/cloud/spanner/internal/spanner_stub.h, line 107 at r2 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

nit: this does not read as part of the "brief" description (which is what the first line in a Doxygen comment becomes). Maybe separate the brief description from the longer description of what channel_id is for?

doxygen ignorance on my part... hopefully better now.

coryan

Reviewed 2 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @scotthart)

…e-cloud-cpp-spanner#1050) Note that `Client` might create multiple channels but the `SessionPool` only uses the first one right now. I'll add proper support for multiple channels in a subsequent change but wanted to get some of the more mechanical parts out of the way.

mr-salty requested review from coryan and scotthart November 13, 2019 21:45

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 13, 2019

coryan suggested changes Nov 13, 2019

View reviewed changes

mr-salty added 2 commits November 14, 2019 13:48

address review comments; set channel_id

bb4a1da

mr-salty commented Nov 14, 2019

View reviewed changes

coryan approved these changes Nov 14, 2019

View reviewed changes

address review comments

0c16ec5

mr-salty commented Nov 14, 2019

View reviewed changes

coryan approved these changes Nov 14, 2019

View reviewed changes

mr-salty merged commit beb6d01 into googleapis:master Nov 14, 2019

mr-salty deleted the multi-stub branch November 14, 2019 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add some plumbing to enable multiple channels #1050

feat: add some plumbing to enable multiple channels #1050

mr-salty commented Nov 13, 2019 •

edited by coryan

Loading

mr-salty commented Nov 13, 2019

coryan left a comment

codecov bot commented Nov 13, 2019 •

edited

Loading

mr-salty left a comment

coryan left a comment

mr-salty left a comment

coryan left a comment

feat: add some plumbing to enable multiple channels #1050

feat: add some plumbing to enable multiple channels #1050

Conversation

mr-salty commented Nov 13, 2019 • edited by coryan Loading

mr-salty commented Nov 13, 2019

coryan left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 13, 2019 • edited Loading

Codecov Report

mr-salty left a comment

Choose a reason for hiding this comment

coryan left a comment

Choose a reason for hiding this comment

mr-salty left a comment

Choose a reason for hiding this comment

coryan left a comment

Choose a reason for hiding this comment

mr-salty commented Nov 13, 2019 •

edited by coryan

Loading

codecov bot commented Nov 13, 2019 •

edited

Loading