fix(bigtable): DataConnection refreshes channels
#9718
Conversation
product-auto-label
bot
added
the
api: bigtable
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov Report
@@ Coverage Diff @@
## main #9718 +/- ##
=======================================
Coverage 93.89% 93.89%
=======================================
Files 1495 1495
Lines 139390 139421 +31
=======================================
+ Hits 130874 130910 +36
+ Misses 8516 8511 -5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
| EXPECT_LE(ns(ms(500)).count(), duration.count()); | ||
| EXPECT_LE(duration.count(), ns(ms(1000)).count()); |
There was a problem hiding this comment.
Does .count() need to be involved? That is, can't we compare the durations directly?
There was a problem hiding this comment.
We could compare the durations directly. I think the idea was to have (arguably) better error messages. e.g.
Expected: (ns(ms(1000)).count()) <= (duration.count()), actual: 1000000000 vs 852000000
vs.
Expected: (ms(1000)) <= (duration), actual: 8-byte object <E8-03 00-00 00-00 00-00> vs 8-byte object <00-8E AB-25 00-00 00-00>
There was a problem hiding this comment.
Fair enough. Perhaps consider these options:
- Leave it as-is.
- Wrap each side of duration expectations in
absl::FromChrono, so you even get even nicer output, including units.
Either of these (.count() or wrapping) could disappear when durations become streamable in C++20.
There was a problem hiding this comment.
I left it as is. I suspect I will need to rewrite the test, anyway. When that happens, I doubt I will use .count() again.
| EXPECT_LE(ns(ms(500)).count(), duration.count()); | ||
| EXPECT_LE(duration.count(), ns(ms(1000)).count()); |
There was a problem hiding this comment.
Fair enough. Perhaps consider these options:
- Leave it as-is.
- Wrap each side of duration expectations in
absl::FromChrono, so you even get even nicer output, including units.
Either of these (.count() or wrapping) could disappear when durations become streamable in C++20.
Fixes #9712
This is mostly a refactor. Update the channel refresh API to accept a
shared_ptr<internal::CompletionQueueImpl>instead of ashared_ptr<CompletionQueue>.Now, we will not back out of the channel refresh loop too early, because the background threads hold onto the CQ object for the lifetime of the stub.
Add a regression test for this behavior that fails at HEAD.
This change is