impl(bigtable): add ChannelUsage class#16031
impl(bigtable): add ChannelUsage class#16031scotthart wants to merge 2 commits intogoogleapis:mainfrom
Conversation
product-auto-label
bot
added
the
api: bigtable
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a new ChannelUsage class for tracking RPC usage, which is a good addition for the upcoming DynamicChannelPool feature. My review focuses on the implementation of this new class. I've identified a potential logic issue in the calculation of the average outstanding RPCs under specific edge conditions and suggest a fix. Additionally, I've pointed out a couple of minor code cleanup opportunities: an unused header and a commented-out line in the test file.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #16031 +/- ##
========================================
Coverage 92.64% 92.64%
========================================
Files 2335 2337 +2
Lines 214792 214928 +136
========================================
+ Hits 198986 199120 +134
- Misses 15806 15808 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| while (iter != measurements_.rend() && iter->timestamp >= window_start) { | ||
| double weight = | ||
| std::chrono::duration<double>(last_time - iter->timestamp).count(); | ||
| last_time = iter->timestamp; | ||
| sum += iter->outstanding_rpcs * weight; | ||
| total_weight += weight; | ||
| ++iter; | ||
| } |
There was a problem hiding this comment.
Would it be possible for all items in measurements_ to be older than window_start? If so, we'd be returning sum=0, but that may not be correctly representing the outstanding rpcs, right? Also, in that case, would erasing the measurements_ be desirable? I'm thinking that may drop the active RPC count.
There was a problem hiding this comment.
Added logic to always keep an old measurement in the deque. This enables computing any missing time from the window that measurements < 60s do not cover. This includes extreme situations like having a constant outstanding rpc level for an hour. Added expectation to test for this.
2 participants
This PR adds the ChannelUsage class which tracks RPC usage over the previous minute on a
T(typically a Stub class). This will be used as part of the upcoming DynamicChannelPool Bigtable feature.