Skip to content

fix: retry upload order #259

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 25 commits into from
Apr 1, 2025
Merged

fix: retry upload order #259

merged 25 commits into from
Apr 1, 2025

Conversation

polbins
Copy link
Contributor

@polbins polbins commented Mar 21, 2025

Summary

After removal of the old retry logic in #258, we're now re-introducing an updated version of retrying in this PR.

We have an updated AnalyticsResponse with a flag to retry when upload failed. The premise is that we will retry when the event file is still present, and won't retry if it's not (e.g. bad event file).

After knowing the result of the response, we'll now retry within the EventPipeline itself through the use of ExponentialBackoffRetryHandler. This class contains the logic for exponential back-off delay that happens every retry attempt.

We've also updated how we're reading the event file from storage. We'll get the sorted event files again, whenever we're retrying.

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?:

@polbins polbins force-pushed the AMP-124458/fix-retry-upload-order branch from bc600a7 to 5fcee0a Compare March 22, 2025 00:13
@polbins polbins force-pushed the AMP-124458/remove-previous-retry-logic branch from 43e7f7d to 6ec357a Compare March 24, 2025 17:58
@polbins polbins force-pushed the AMP-124458/fix-retry-upload-order branch 4 times, most recently from bfad97e to 3e3ad04 Compare March 25, 2025 16:34
@polbins polbins marked this pull request as ready for review March 25, 2025 16:50
@polbins polbins requested review from igor-amp and sojingle March 25, 2025 16:55
@polbins polbins force-pushed the AMP-124458/fix-retry-upload-order branch from 3e3ad04 to e893149 Compare March 25, 2025 20:43
@polbins polbins force-pushed the AMP-124458/fix-retry-upload-order branch from e893149 to a34ce24 Compare March 26, 2025 21:48
Copy link
Contributor

@igor-amp igor-amp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a bit overcomplicated logic with a bug in reset.

@polbins polbins force-pushed the AMP-124458/fix-retry-upload-order branch from a34ce24 to 498a4f0 Compare March 26, 2025 21:59
@polbins polbins requested a review from igor-amp March 27, 2025 18:25
@polbins polbins force-pushed the AMP-124458/fix-retry-upload-order branch 4 times, most recently from 63c8002 to 1cc8aa1 Compare March 28, 2025 23:25
Copy link
Contributor

@igor-amp igor-amp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@polbins resolve comments and let's merge

Base automatically changed from AMP-124458/remove-previous-retry-logic to main April 1, 2025 23:07
@polbins polbins force-pushed the AMP-124458/fix-retry-upload-order branch from a9a46ac to 445aadb Compare April 1, 2025 23:08
@polbins polbins merged commit 59a339d into main Apr 1, 2025
4 checks passed
@polbins polbins deleted the AMP-124458/fix-retry-upload-order branch April 1, 2025 23:19
github-actions bot pushed a commit that referenced this pull request Apr 2, 2025
## [1.20.5](v1.20.4...v1.20.5) (2025-04-02)

### Bug Fixes

* compose autocapture properties update ([#261](#261)) ([a49b6a7](a49b6a7))
* reformatting and warning fixes for upload ordering ticket  ([#257](#257)) ([61eb7ee](61eb7ee))
* retry upload order ([#259](#259)) ([59a339d](59a339d))
Copy link

github-actions bot commented Apr 2, 2025

🎉 This PR is included in version 1.20.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants