Skip to content

Conversation

@ifel
Copy link
Contributor

@ifel ifel commented Oct 16, 2019

If update fails, the whole run fails. Try 5 times with delays 2 ^ retry_attempt between tries.


def update
@cmd.pull.stdout
retry_count = 0
Copy link
Collaborator

@jaymzh jaymzh Oct 16, 2019

Choose a reason for hiding this comment

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

I don't inherently have a problem with the idea, but I have a few concerns with the implementation:

  1. Most software that acts as a client does some sort of exponential backoff, so us doing it is fine, but also, some of the clients we're calling here probably also do this internally or might in the future, and double-exponentially-backing-off is likely to paper over problems and thus hide real infrastructure issues from those people who care about their stuff working. So, we should definitely make this configurable, with the default being either no retries or a single retry. I recommend the config keys being retries and retry_backoff_interval or some such. It also needs to log when this is happening so people know.

  2. This copy-pasta is sad-making, we can do better. One, I suggest a function in the super class that does something like:

def retriable(name, max_retries, backoff)
  retry_count
  begin
    yield
  rescue StandardError => e
    @logger.error("Something went wrong runing #{name}!")
    @logger.error(e)
    if retry_count > max_retries
      raise
    end
    retry_count += 1
    @logger.warn("Retrying #{name}: retry #{retry_count}")
    sleep(2 ** retry_backoff)
    retry
  end
end

And then you can use that from wherever like:

retriable("update repo", desired_retries, desired_backoff) do
  @cmd.pull.stdout
end
  1. Further, this update and HG's update are the same, and simple, so I suggest we make the super class have this, and we can override it from SVN's class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants