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

Conversation

@mr-salty
Copy link
Contributor

@mr-salty mr-salty commented Feb 13, 2020

fixes #1250


This change is Reviewable

@mr-salty mr-salty requested review from coryan, devbww and devjgm February 13, 2020 01:40
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 13, 2020
@codecov
Copy link

codecov bot commented Feb 13, 2020

Codecov Report

Merging #1262 into master will decrease coverage by 0.01%.
The diff coverage is 50%.

Impacted file tree graph

@@ Coverage Diff @@ ## master #1262 +/- ## ========================================== - Coverage 94.87% 94.85% -0.02%  ========================================== Files 185 181 -4 Lines 13818 13771 -47 ========================================== - Hits 13110 13063 -47  Misses 708 708
Impacted Files Coverage Δ
google/cloud/spanner/connection_options.h 100% <ø> (ø) ⬆️
google/cloud/spanner/connection_options.cc 91.89% <50%> (ø) ⬆️
...on_tests/rpc_failure_threshold_integration_test.cc 85.71% <0%> (-2.07%) ⬇️
...ud/spanner/integration_tests/client_stress_test.cc 81.57% <0%> (-1.76%) ⬇️
google/cloud/spanner/client.cc 97.08% <0%> (-0.73%) ⬇️
google/cloud/spanner/value.h 92.8% <0%> (-0.11%) ⬇️
...anner/integration_tests/client_integration_test.cc 98.47% <0%> (-0.01%) ⬇️
.../spanner/benchmarks/multiple_rows_cpu_benchmark.cc 92.36% <0%> (ø) ⬆️
google/cloud/spanner/samples/samples.cc 88.04% <0%> (+0.47%) ⬆️

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 4b648a0...f8e8d8d. Read the comment docs.

Copy link
Contributor

@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! A couple of administrative changes:

  • This is a breaking change and should be labeled as so (refactor!:) in the commit message.
  • You also need to update the API baseline.

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

@mr-salty mr-salty changed the title refactor: use BackgroundThreads from common refactor!: use BackgroundThreads from common Feb 13, 2020
@mr-salty
Copy link
Contributor Author

Done (updated description and API dump).

@coryan
Copy link
Contributor

coryan commented Feb 13, 2020

Unfortunately @devjgm merged changes to the API dump, you need to rebase and so forth. It looks good though, ready to approve once the builds pass.

Copy link
Contributor

@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:

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

@mr-salty mr-salty merged commit 9574fca into googleapis:master Feb 13, 2020
@mr-salty mr-salty deleted the background branch February 13, 2020 20:46
devjgm pushed a commit to devjgm/google-cloud-cpp that referenced this pull request May 7, 2020
…loud-cpp-spanner#1262) * refactor: use `BackgroundThreads` from `common` fixes googleapis/google-cloud-cpp-spanner#1250 * update API/ABI after rebase
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.

3 participants