Skip to content

Conversation

felipeamarante
Copy link

@felipeamarante felipeamarante commented Nov 24, 2017

Hi guys,

Faced a few issues whenever connection to s3 drops by unexpected reason and installation remains hung. Added a slight retry attempt in case of network being disrupted , over-utilized or any packet drops happen. - Issue #95

Please feel free to contribute or to let me know further enhancements.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.567% when pulling 9dcf27c on felipeamarante:install-retry-from-s3 into 91b4466 on aws:master.

Copy link

@mmerkes mmerkes left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I like the addition of a retry here and it would likely help for intermittent issues. See my comment below, and let me know if you'd like to update your PR or you'd like use to make the change.

bin/install Outdated
end
rescue OpenURI::HTTPError => e
@log.error("Could not find package to download at '#{uri.to_s}'")
retry if (retries +=1) < 5
Copy link

Choose a reason for hiding this comment

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

I think adding a retry here is a good idea. However, I think we might want to address a couple considerations:

  1. The error that you mention in your ticket is a OpenSSL::SSL::SSLError, which would not be handled here. It will just jump out of here anyway, so we probably want to add that to address the specific case.
  2. Retries are a good addition to this call, but it would be best if we added exponential backoff to the retry to get the best results, which will give us more opportunities for success in cause the issue lasts for 5 rapid fire calls.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.8%) to 92.779% when pulling 7ee90c5 on felipeamarante:install-retry-from-s3 into 91b4466 on aws:master.

@felipeamarante
Copy link
Author

Hey, suggestions were updated on the PR code. Thanks for the feedback 👍

@rohkat-aws
Copy link
Contributor

merged in a diff pull request #163

@rohkat-aws rohkat-aws closed this May 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants