Skip to content

Add a backoff and retry to S3 puts to help with Slow Down messages.#321

Merged
zerebubuth merged 4 commits intomasterfrom
zerebubuth/backoff-and-retry-s3-puts
Dec 21, 2017
Merged

Add a backoff and retry to S3 puts to help with Slow Down messages.#321
zerebubuth merged 4 commits intomasterfrom
zerebubuth/backoff-and-retry-s3-puts

Conversation

@zerebubuth
Copy link
Copy Markdown
Member

No description provided.

Copy link
Copy Markdown
Member

@rmarianski rmarianski left a comment

Choose a reason for hiding this comment

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

Are you sure that the retry will actually catch the exception? I think it won't end up working that way. Here's a simple example:

def retry():
    for _ in range(3):
        try:
            print 'yielding'
            yield
            print 'break'
            break
        except Exception:
            print 'caught exception'
    else:
        print 'else'
        yield


for _ in retry():
    raise Exception('foo')

Comment thread tilequeue/store.py Outdated
yield
break

except Exception as e:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Interesting idea to use a generator, but I don't think that it'll actually catch the exception here and retry.

@zerebubuth
Copy link
Copy Markdown
Member Author

Are you sure that the retry will actually catch the exception? I think it won't end up working that way.

Oooops. Wow, I hadn't realised that yield wasn't in the path to catch exceptions. Thanks for pointing that out.

I just pushed a different, and sadly more complex, version in 2520055. Please could you have a look and let me know if you think that's better?

Copy link
Copy Markdown
Member

@iandees iandees left a comment

Choose a reason for hiding this comment

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

Looks good -- it'd be nice to add a logger as mentioned but not required.

Comment thread tilequeue/store.py Outdated
reduced_redundancy=self.reduced_redundancy,
)

@_backoff_and_retry(Exception)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It'd be nice if there was a logger we could pass in so we know when things retry rather than stuff just silently taking forever.

@zerebubuth
Copy link
Copy Markdown
Member Author

@iandees, @rmarianski does that look good with the logger threaded through?

@rmarianski
Copy link
Copy Markdown
Member

@iandees, @rmarianski does that look good with the logger threaded through?

LGTM 👍

@zerebubuth zerebubuth merged commit ee9498d into master Dec 21, 2017
@zerebubuth zerebubuth deleted the zerebubuth/backoff-and-retry-s3-puts branch December 21, 2017 18:35
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.

3 participants