Repository navigation
Conversation
DynamicChannelPool::EvictBadChannels previously swapped unhealthy channels with tail elements without bounding the reverse scan, which left moved-from nullptr entries in channels_ and dropped healthy channels whenever a bad channel occupied a tail slot, and skipped moving channels to draining_channels_ when all channels were unhealthy. In addition, HandleBadChannels unconditionally checked the second candidate iterator even in a 1-channel pool, causing a singular iterator push and double-pop. Guard the candidate iterators by pool size in HandleBadChannels and move unhealthy channels directly into draining_channels_ followed by std::remove/erase compaction in EvictBadChannels.
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
product-auto-label
Bot
added
the
api: bigtable
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the bad channel handling logic in DynamicChannelPool to safely and cleanly evict unhealthy channels. Specifically, it adds bounds checks before finding good channels, simplifies the eviction process by moving bad channels to draining_channels_ and erasing null pointers from channels_ using std::remove, and updates the test suite with new test cases to verify correct eviction behavior in various pool sizes. I have no additional feedback to provide as there are no review comments to assess.
scotthart
added
the
do not merge
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16650 +/- ##
==========================================
- Coverage 92.53% 92.49% -0.04%
==========================================
Files 2269 2269
Lines 222896 223132 +236
==========================================
+ Hits 206250 206395 +145
- Misses 16646 16737 +91 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
DynamicChannelPool::EvictBadChannelsso unhealthy channels are moved directly intodraining_channels_and compacted out ofchannels_viastd::remove/eraseinstead of swapping with an unbounded reverse iterator and poppingbad_channel_iters.size()elements.nullptrentries inchannels_, accidentally popping healthy channels when a bad channel is near the back ofchannels_, or failing to drain unhealthy channels when all channels in the pool are unhealthy.FindGoodChannelcandidate checks inHandleBadChannelsbyd.iterators.size()so 1-channel pools do not inspect or pushd.channel_2_iter.Testing
google/cloud/bigtable/internal/dynamic_channel_pool_test.cccovering tail-slot bad channel eviction, 1-channel pool eviction & replacement, and all-channels-bad eviction & recovery.bazel test //google/cloud/bigtable:internal_dynamic_channel_pool_test.