fix(flagd): back off before stream reconnect - #391
Conversation
![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 _shutdown_event using threading.Event in both GrpcResolver and grpc_watcher to manage reconnection delays and facilitate cleaner shutdowns. It refactors the listen method in GrpcResolver into smaller helper methods and adds comprehensive unit tests for the reconnection and backoff logic. The reviewer suggests using the initial backoff duration rather than the maximum backoff for reconnection attempts to prevent excessive delays during recovery from transient failures.
| logger.debug(self.config.fatal_status_codes) | ||
|
|
||
| self.retry_grace_period = config.retry_grace_period | ||
| self.retry_backoff_max_seconds = config.retry_backoff_max_ms * 0.001 |
There was a problem hiding this comment.
Using the maximum backoff duration (retry_backoff_max_ms) as a constant delay for every reconnect attempt can lead to significant delays in recovery from transient failures. It is generally better to use the initial backoff duration (retry_backoff_ms) or implement an exponential backoff strategy. Since gRPC already handles connection-level backoff internally, a small application-level delay is sufficient to prevent tight loops while allowing faster recovery.
| self.retry_backoff_max_seconds = config.retry_backoff_max_ms * 0.001 | |
| self.retry_backoff_seconds = config.retry_backoff_ms * 0.001 | |
| self.retry_backoff_max_seconds = config.retry_backoff_max_ms * 0.001 |
| ) | ||
|
|
||
| def _wait_before_reconnect(self) -> None: | ||
| self._shutdown_event.wait(self.retry_backoff_max_seconds) |
There was a problem hiding this comment.
As mentioned in the initialization, using the maximum backoff for every retry significantly impacts availability. Consider using the initial backoff duration instead. Note that you will also need to update the corresponding regression tests in tests/test_grpc_resolver.py.
| self._shutdown_event.wait(self.retry_backoff_max_seconds) | |
| self._shutdown_event.wait(self.retry_backoff_seconds) |
| return False | ||
|
|
||
| def _wait_before_reconnect(self) -> None: | ||
| self._shutdown_event.wait(self.retry_backoff_max_seconds) |
There was a problem hiding this comment.
Using the maximum backoff duration (retry_backoff_max_ms) as a constant delay for every reconnect attempt can lead to significant delays in recovery from transient failures. It is generally better to use the initial backoff duration (retry_backoff_ms) or implement an exponential backoff strategy. Since gRPC already handles connection-level backoff internally, a small application-level delay is sufficient to prevent tight loops while allowing faster recovery. Note that you will also need to update the corresponding regression tests in tests/test_grpc_watcher.py.
| self._shutdown_event.wait(self.retry_backoff_max_seconds) | |
| self._shutdown_event.wait(self.retry_backoff_seconds) |
|
Thanks for the review. I kept this tied to This is separate from gRPC's RPC-level retry behavior; the added wait only applies between one stream ending and the next stream being created. That seems to align with the issue/spec and the reconnect-loop scenario they call out. |
Signed-off-by: looooown2006 <102905927+looooown2006@users.noreply.github.com>
|
@coderabbitai /review |
|
✅ Action performedReview finished.
|
7282f02 to
4f5c496
Compare
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| def _wait_before_reconnect(self) -> None: | ||
| self._shutdown_event.wait(self.retry_backoff_max_seconds) | ||
|
|
||
| def _handle_rpc_error(self, e: grpc.RpcError) -> bool: |
There was a problem hiding this comment.
Thanks for the method extractions.
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
3869a48 to
23ace6e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #391 +/- ##
==========================================
- Coverage 95.55% 94.20% -1.35%
==========================================
Files 24 31 +7
Lines 1034 1070 +36
==========================================
+ Hits 988 1008 +20
- Misses 46 62 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4 participants
Summary
Fixes the flagd provider stream reconnect loop so both gRPC stream implementations apply an application-level delay before creating the next stream after a stream error or normal stream completion.
Changes:
GrpcResolvernow waitsretry_backoff_max_msbefore recreating the RPCEventStream.GrpcWatchernow waitsretry_backoff_max_msbefore recreating the in-processSyncFlagsstream.Closes #390.
Verification
Note: I also tried the broader non-e2e pytest command, but the local run still initialized the Docker-backed e2e test-harness fixture and failed before running those cases because Docker Desktop's Linux engine was not running on this Windows machine.