-
Notifications
You must be signed in to change notification settings - Fork 134
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
If the app was created instead of fetched, perform upload in a retry loop. #164
base: master
Are you sure you want to change the base?
If the app was created instead of fetched, perform upload in a retry loop. #164
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this contribution! I believe we should be careful with adding a new dependency here.
Also, this will require new unit test.
each_retry = Proc.new do |exception, try, elapsed_time, next_interval| | ||
UI.message("Release upload failed (#{exception.message}), retrying. Attempt# #{try}. #{next_interval} seconds until the next try.") | ||
end | ||
Retriable.retriable(tries: 60, on_retry: each_retry) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to add a new dependency, but it's not added to Gemfile. Retriable is also not a direct dependency in fastlane.
self.run_dsym_upload(params) unless upload_mapping_only || upload_build_only | ||
self.run_mapping_upload(params) unless upload_dsym_only || upload_build_only | ||
else | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not seem to be a case that we should reach.
@jp-andre Hi Jean-Philippe, I opened this more to point out the problem identified by #151 and a potential resolution. I'm not convinced that having an exponential backoff retry coded into the client is a great fix. Is this something that the appcenter backend team should look at before we attempt to hack around the app creation readiness latency? I'm more than happy to fix this up if you want to move forward with it. I had noticed that the appcenter plugin has no 3rd party gem dependencies and assumed that was a conscious decision. If you want to move forward with this PR would you prefer the addition of the Retriable gem or an internal implementation of some form of retry? |
Hi @krimsonkla -- this PR was totally forgotten, I'm sorry about that. |
Keeping this opened, as I think the approach is acceptable... better than nothing :) |
This is my response to issue #151
My experience with this issue has been that if the plugin attempts to perform a release upload directly after creating the app, it will fail for some time. Apparently there is a background task or data replication within appcenter that must occur before the upload will complete.
This may need some fine tuning on the retry loop logic in order to avoid lengthy delays. At the moment it is using a randomized exponential backoff to prevent flooding the system. I feared this issue may also be throttling related and wanted to reduce the number of API hits.
Here are some sample uploads with debug messages. I included 3 simply to show the time range variance.