Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
Add retry mechanism for failed media uploads in
MediaManagerwith tests for retryable and non-retryable HTTP statuses.MediaManager._upload_with_status_checkfunction inmedia_manager.pyto handle upload and status check._process_upload_media_jobto use_upload_with_status_checkwith backoff strategy.test_media_upload_retries_on_retryable_http_statusintest_media_manager.pyto verify retry on 503 status.test_media_upload_gives_up_on_non_retryable_http_statusintest_media_manager.pyto verify no retry on 403 status.This description was created by
for bc20c1a. You can customize this summary. It will automatically update as commits are pushed.
Disclaimer: Experimental PR review
Greptile Summary
Added retry mechanism for failed media uploads with proper error tracking. The implementation wraps the upload operation in
_upload_with_status_checkwhich callsraise_for_status()to ensure HTTP errors are properly caught. Failed uploads are now logged via themedia.patchAPI before the exception is re-raised, ensuring visibility into upload failures even when retries are exhausted._upload_with_status_checkfunction that performs the upload and validates HTTP statusmedia.patchAPI with status code, error message, and timing_request_with_backoffretry logic which retries 5xx errors and 429, but gives up on other 4xx errorsConfidence Score: 5/5
_request_with_backoffmechanism which already has proper retry/giveup logic for different HTTP status codes. The addition of error logging viamedia.patchensures visibility into upload failures. The comprehensive tests verify both retryable and non-retryable scenarios, confirming the implementation works as expected.Important Files Changed
media.patchAPI callFlowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Start Upload] --> B[Call _upload_with_status_check] B --> C[HTTP PUT to upload_url] C --> D[Call raise_for_status] D --> E{Status OK?} E -->|2xx| F[Return Response] E -->|Error| G[Raise HTTPStatusError] G --> H{Retryable?} H -->|5xx or 429| I[Backoff Retry] I --> B H -->|4xx except 429| J[Catch in Exception Handler] F --> K[Log Success via media.patch] J --> L[Log Failure via media.patch] L --> M[Re-raise Exception] K --> N[Complete] M --> O[Error Logged & Propagated]Last reviewed commit: bc20c1a