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

feat: use 1 client with multiple channels for the benchmark #1194

Merged
merged 1 commit into from
Jan 16, 2020
Merged

feat: use 1 client with multiple channels for the benchmark #1194

merged 1 commit into from
Jan 16, 2020

Conversation

mr-salty
Copy link
Contributor

@mr-salty mr-salty commented Jan 15, 2020

  • factor Client creation code out to a helper
  • make InsertOrUpdateExperiment use DefaultPRNG like the others
  • remove superfluous mutex from UpdateDmlExperiment::RunIteration
  • start to migrate from minimum/maximum_clients to _channels

Part of #1193


This change is Reviewable

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 15, 2020
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.

Thanks, it looks good, assuming I understand the plan: after this you will change the other benchmarks and then rename the *_clients variables?

Somewhat unrelated: can you attach your results to one of the bugs so we know (in the future) why keeping a single client was enough?

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

@codecov
Copy link

codecov bot commented Jan 15, 2020

Codecov Report

Merging #1194 into master will decrease coverage by 0.08%.
The diff coverage is 86.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1194      +/-   ##
==========================================
- Coverage    95.7%   95.62%   -0.09%     
==========================================
  Files         174      174              
  Lines       13601    13565      -36     
==========================================
- Hits        13017    12971      -46     
- Misses        584      594      +10
Impacted Files Coverage Δ
...nner/benchmarks/single_row_throughput_benchmark.cc 91.93% <100%> (-0.81%) ⬇️
...ogle/cloud/spanner/benchmarks/benchmarks_config.cc 93.51% <25%> (-5.51%) ⬇️
...ud/spanner/integration_tests/client_stress_test.cc 81.57% <0%> (-1.76%) ⬇️
...loud/spanner/internal/partial_result_set_source.cc 93.24% <0%> (-1.36%) ⬇️
google/cloud/spanner/internal/spanner_stub.cc 69.23% <0%> (-1.1%) ⬇️
...on_tests/rpc_failure_threshold_integration_test.cc 85.55% <0%> (-0.16%) ⬇️
...anner/integration_tests/client_integration_test.cc 98.47% <0%> (-0.01%) ⬇️
google/cloud/spanner/keys.h 100% <0%> (ø) ⬆️
.../spanner/benchmarks/multiple_rows_cpu_benchmark.cc 92.35% <0%> (ø) ⬆️

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 27048c7...5e75b25. Read the comment docs.

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.

yeah, I'm planning to do the multiple row benchmark in a different PR then make those channels.

I added a graph to https://github.com/googleapis/google-cloud-cpp-spanner/issues/1193

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @devbww)

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.

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @devbww)

@mr-salty mr-salty merged commit b5f8413 into googleapis:master Jan 16, 2020
@mr-salty mr-salty deleted the bmtest branch January 16, 2020 18:02
devjgm pushed a commit to devjgm/google-cloud-cpp that referenced this pull request May 7, 2020
…is/google-cloud-cpp-spanner#1194)

* feat: use 1 client with multiple channels for the benchmark

Part of googleapis/google-cloud-cpp-spanner#1193
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