Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

feat: update the pool options to match the design #1089

Merged
merged 4 commits into from
Nov 21, 2019
Merged

feat: update the pool options to match the design #1089

merged 4 commits into from
Nov 21, 2019

Conversation

mr-salty
Copy link
Contributor

@mr-salty mr-salty commented Nov 19, 2019

  • max_sessions is now max_sessions_per_channel
  • we're not going to be using write_sessions_fraction, so remove it.

Note I also flipped the precedence of min_sessions and (max_sessions_per_channel * the number of channels) - the latter now takes precedence, just for simplicity; I don't think it matters that much because it's a misconfiguration.

Part of #307


This change is Reviewable

* `max_sessions` is now `max_sessions_per_channel`
* we're not going to be using `write_sessions_fraction`, so remove it.

Part of #307
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 19, 2019
@codecov
Copy link

codecov bot commented Nov 19, 2019

Codecov Report

Merging #1089 into master will increase coverage by 0.07%.
The diff coverage is 95.65%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1089      +/-   ##
==========================================
+ Coverage    91.6%   91.67%   +0.07%     
==========================================
  Files         164      164              
  Lines       12116    12171      +55     
==========================================
+ Hits        11099    11158      +59     
+ Misses       1017     1013       -4
Impacted Files Coverage Δ
google/cloud/spanner/internal/session_pool.h 100% <ø> (ø) ⬆️
google/cloud/spanner/internal/session_pool_test.cc 99.39% <100%> (-0.01%) ⬇️
google/cloud/spanner/internal/session_pool.cc 95.83% <92.3%> (-1.65%) ⬇️
google/cloud/spanner/client.cc 82.69% <0%> (-0.97%) ⬇️
...on_tests/rpc_failure_threshold_integration_test.cc 85.55% <0%> (-0.16%) ⬇️
...anner/integration_tests/client_integration_test.cc 99.45% <0%> (-0.01%) ⬇️
google/cloud/spanner/keys.h 100% <0%> (ø) ⬆️
.../cloud/spanner/internal/background_threads_test.cc 100% <0%> (ø) ⬆️
google/cloud/spanner/internal/spanner_stub.cc 69.23% <0%> (ø) ⬆️
... and 3 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 150b849...77d7cbc. Read the comment docs.

Copy link
Member

@coryan coryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @devbww, @devjgm, and @mr-salty)


google/cloud/spanner/internal/session_pool.h, line 53 at r1 (raw file):

  // The maximum number of sessions to create on each channel.
  // Values <= 1 are treated as 1.
  // The `max_sessions_per_channel` limit takes precedence over `min_sessions`.

FYI: I had to read the implementation and this comment like 3 times before I could figure out what "take precedence" meant. At first I thought it mean "the number of sessions can be smaller than min_sessions if max_sessions_per_channel*num_channels < min_session"? But I think it means (the current implementation) "min_sessions can never be smaller than max_sessions_per_channel * num_channels"


google/cloud/spanner/internal/session_pool.h, line 144 at r1 (raw file):

  std::unique_ptr<BackoffPolicy const> backoff_policy_prototype_;
  SessionPoolOptions const options_;
  const int max_pool_size_;

s/const int/int const/

Copy link
Contributor Author

@mr-salty mr-salty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


google/cloud/spanner/internal/session_pool.h, line 53 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

FYI: I had to read the implementation and this comment like 3 times before I could figure out what "take precedence" meant. At first I thought it mean "the number of sessions can be smaller than min_sessions if max_sessions_per_channel*num_channels < min_session"? But I think it means (the current implementation) "min_sessions can never be smaller than max_sessions_per_channel * num_channels"

I reworded it; hopefully it's clearer. It means min_sessions can never be larger than max_sessions_per_channel * num_channels


google/cloud/spanner/internal/session_pool.h, line 144 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

s/const int/int const/

Done.

Copy link
Member

@coryan coryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @devbww and @devjgm)

@mr-salty mr-salty merged commit e1bcbf7 into googleapis:master Nov 21, 2019
@mr-salty mr-salty deleted the options branch November 21, 2019 00:34
devjgm pushed a commit to devjgm/google-cloud-cpp that referenced this pull request May 7, 2020
…cloud-cpp-spanner#1089)

* `max_sessions` is now `max_sessions_per_channel`
* we're not going to be using `write_sessions_fraction`, so remove it.

Part of googleapis/google-cloud-cpp-spanner#307
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants