Retry logic improvements + Fix Tests warnings #50
Conversation
- Add transient HTTP status code retries (429, 502, 503, 504) - Implement Retry-After header support for rate limiting compliance - Add exponential backoff with jitter to prevent thundering herd - Create centralized retry configuration in DataverseConfig - Add metadata-specific retry logic for propagation delays - Include comprehensive test coverage (23 new tests) - Maintain backwards compatibility with existing configurations
- Resolved conflicts in http.py docstring - Maintained enhanced retry functionality with updated documentation
- Fix DeprecationWarning by replacing datetime.utcnow() with datetime.now(UTC) - Fix PytestCollectionWarning by renaming test helper classes to avoid pytest auto-discovery - Rename TestClient -> MockHttpClient and TestableClient -> MockLogicalCrudClient - Achieve 100% warning reduction (13 -> 0 warnings) with no functionality changes - All 48 tests continue to pass with faster execution time
- Remove RETRY_LOGIC_SUMMARY.md as it was intended for development documentation only - Keep the SDK focused on production code and tests
There was a problem hiding this comment.
Pull Request Overview
This PR introduces comprehensive retry logic improvements to the PowerPlatform Dataverse Python SDK, enhancing resilience through exponential backoff, jitter, transient error handling, and metadata-specific retry logic. The changes improve production reliability while maintaining backwards compatibility through careful use of getattr for new configuration options.
Key Changes:
- Added configurable retry behavior with exponential backoff, jitter, and HTTP status code retries (429, 502, 503, 504) to
HttpClient - Centralized metadata request retry logic into
_request_metadata_with_retrymethod to handle 404 errors during metadata propagation - Extended
DataverseConfigwith new retry parameters (http_max_backoff,http_jitter,http_retry_transient_errors) with sensible defaults
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
src/PowerPlatform/Dataverse/core/http.py |
Enhanced HTTP client with exponential backoff, jitter, Retry-After header support, and transient status code retry logic |
src/PowerPlatform/Dataverse/core/config.py |
Added new configuration fields for retry behavior with updated documentation and defaults |
src/PowerPlatform/Dataverse/core/errors.py |
Updated timestamp generation to use timezone-aware UTC datetime |
src/PowerPlatform/Dataverse/data/odata.py |
Refactored metadata retry logic into centralized helper method and integrated new retry configuration options |
tests/unit/core/test_http_retry_logic.py |
Comprehensive test coverage for new HTTP retry logic (15+ test cases) |
tests/unit/data/test_metadata_retry_logic.py |
Test coverage for metadata-specific retry behavior (6 test cases) |
tests/unit/core/test_http_errors.py |
Updated test client class name from TestClient to MockHttpClient for clarity |
tests/unit/data/test_logical_crud.py |
Updated test client class name from TestableClient to MockLogicalCrudClient for clarity |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Replace datetime.UTC (Python 3.11+) with datetime.timezone.utc (Python 3.10+) - Maintains compatibility with project's requires-python >= 3.10 requirement - All tests passing (48/48)
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add explicit 'return None' to methods with -> None annotation for clarity - Fix _update, _delete, _delete_table methods in odata.py - Fix update method in pandas_adapter.py (bare return -> return None) - Fix delete_table method in client.py - Fix critical HTTP retry bug: add missing 'continue' statement for transient status retries - Ensures consistent return patterns and proper retry behavior for 429/502/503/504 errors - All 48 tests pass
- Rename single-letter test mock classes to descriptive names (PEP 8 compliance): * T -> MockToken (5 test files) * R -> MockResponse (1 test file) - Remove unused 'import time' statements from example files - Improves code readability and follows Python naming conventions - All 48 tests continue to pass
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
src/PowerPlatform/Dataverse/data/odata.py:932
- Variable last_error is not used.
last_error = err
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
src/PowerPlatform/Dataverse/data/odata.py:932
- Variable last_error is not used.
last_error = err
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Return Pattern Refinements: - Keep explicit 'return None' only for early exits (good practice) - Remove redundant explicit returns at end of functions (idiomatic Python) Documentation Improvements: - Fix _calculate_retry_delay example to use correct parameter names (backoff vs base_delay) - Remove Mock dependency from example code for better user experience - Provide self-contained, runnable examples that users can actually execute - Examples now demonstrate exponential backoff behavior clearly All 48 tests continue to pass with refined patterns.
| def __init__( | ||
| self, | ||
| *, | ||
| retries: Optional[int] = None, |
There was a problem hiding this comment.
why is default value added in code but not in signatures?
| self.jitter = jitter | ||
| self.retry_transient_errors = retry_transient_errors | ||
|
|
||
| # Transient HTTP status codes that should be retried |
There was a problem hiding this comment.
we should probably use the TRANSIENT_STATUS defined in error_codes.py for this
| self.config = config or __import__("PowerPlatform.Dataverse.core.config", fromlist=["DataverseConfig"]).DataverseConfig.from_env() | ||
| self._http = HttpClient( | ||
| retries=self.config.http_retries, | ||
| backoff=self.config.http_backoff, |
There was a problem hiding this comment.
should we unify the format used here?
| :raises HttpError: If the request fails after all retry attempts. | ||
| """ | ||
| last_error = None | ||
| for attempt in range(3): # 3 attempts for metadata operations |
There was a problem hiding this comment.
this would be on top of the http retries we already have, so by default could retry 3 * 5 times? is this intended?
This pull request implements comprehensive, enterprise-grade retry logic for the PowerPlatform Dataverse Python SDK, greatly improving resilience, reliability, and configurability for production environments. The changes introduce transient HTTP error handling, rate limit compliance, exponential backoff with jitter, centralized configuration, and metadata-specific retry logic, while maintaining backwards compatibility and expanding test coverage.
Retry Logic Enhancements
src/PowerPlatform/Dataverse/core/http.pynow retries on transient HTTP status codes (429, 502, 503, 504), respectsRetry-Afterheaders, applies exponential backoff with jitter, and allows configuration of max backoff and retry behavior.This pull request enhances the HTTP resilience and retry logic for Dataverse operations, making the client more robust against transient errors and network issues. The main changes include configurable retry behavior (with exponential backoff, jitter, and support for HTTP status code retries), improved metadata request reliability, and updates to related configuration and tests.
HTTP Retry & Resilience Improvements
DataverseConfigfor HTTP retries, backoff, max backoff, timeout, jitter, and transient error retries, with improved docstrings and defaults.HttpClientto support exponential backoff, jitter, capped delays, and automatic retries for transient HTTP status codes (429, 502, 503, 504), including support for theRetry-Afterheader. [1] [2]ODataClientto pass new retry-related config options toHttpClient.Metadata Request Reliability
ODataClientto use a new_request_metadata_with_retryhelper, which retries 404 errors with exponential backoff, improving reliability after schema changes. [1] [2] [3]Configuration Defaults
DataverseConfig.from_envto clarify that unset options will use sensible defaults inHttpClient, improving maintainability.Other Improvements
randomimport for jitter logic.Testing Updates
MockHttpClientand updated references in unit tests for clarity. [1] [2] [3] [4] [5]Centralized and Configurable Retry Settings
DataverseConfiginsrc/PowerPlatform/Dataverse/core/config.pyadds new fields for retry attempts, backoff, max backoff, timeout, jitter, and transient error retry, with defaults and environment-independent configuration.Metadata-Specific Retry Logic
src/PowerPlatform/Dataverse/data/odata.pynow use a centralized_request_metadata_with_retry()method to handle 404 errors due to propagation delays, removing duplicate retry code and improving reliability for schema changes.Backwards Compatibility and Integration
getattr()for new config fields to avoid breaking existing code, defaults to safe values, and ensures all existing tests pass.Test Coverage and Minor Improvements
These changes deliver enterprise-grade retry resilience, better error classification, and flexible configuration, ensuring Dataverse SDK reliability.