Summary

Retrying when we encounter a connection reset

Review Request #3563 — Created Nov. 26, 2012 and discarded April 23, 2014, 3:58 p.m.

Information

Owner

atagar

Repository

RBTools

Branch

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

Our users have occasionally encountered connection resets when running postreview.py from remote locations with poor connectivity. These errors are transient, so adding retries when postreview encounters a timeout.

This is available in my timeout_retries branch...

https://github.com/atagar/rbtools/commit/08c288484576503768c29e87c9b6cb08529fec90
https://github.com/atagar/rbtools/tree/timeout_retries

Testing Done

This exact change is untested, but we've been running an identical change with RBTools 0.3 for three months now without any issues.

Issues

Description	From	Last Updated
Col: 80 E501 line too long (87 > 79 characters)	reviewbot	May 10, 2013, 1:40 p.m.
Col: 80 E501 line too long (108 > 79 characters)	reviewbot	May 10, 2013, 1:40 p.m.
Col: 80 E501 line too long (100 > 79 characters)	reviewbot	May 10, 2013, 1:40 p.m.
Col: 80 E501 line too long (95 > 79 characters)	reviewbot	May 10, 2013, 1:40 p.m.
Col: 16 E111 indentation is not a multiple of four	reviewbot	May 10, 2013, 4:42 p.m.
Col: 16 E113 unexpected indentation	reviewbot	May 10, 2013, 4:42 p.m.
Col: 18 E111 indentation is not a multiple of four	reviewbot	May 10, 2013, 4:44 p.m.
Col: 18 E113 unexpected indentation	reviewbot	May 10, 2013, 4:44 p.m.
Col: 17 E901 IndentationError	reviewbot	May 10, 2013, 4:44 p.m.
Col: 17 E113 unexpected indentation	reviewbot	May 10, 2013, 4:50 p.m.
Col: 17 E112 expected an indented block	reviewbot	May 10, 2013, 4:50 p.m.
Please add a docstring for this method.	david	Aug. 20, 2013, 1:05 a.m.
Instead of using the continuation character \, please wrap the condition in parentheses.	david	Aug. 20, 2013, 1:05 a.m.
Why the sleep? This should also probably have a handler for the case where tries == 0, and show an …	david	Aug. 20, 2013, 1:05 a.m.

This and my other RBTools patches (http://reviews.reviewboard.org/r/3221/ and http://reviews.reviewboard.org/r/3208/) have collected dust for three to seven months. Is RBTools no longer maintained?

chipx86 March 7, 2013, 5:04 p.m.

RBTools is being maintained, but it's been undergoing a significant rewrite. There will be a 0.5 release out soon that drastically changes the codebase. One of the main things is that communication code is now common and not isolated to postreview.py (though postreview.py is staying around mostly as-is for backwards-compatibility reasons for another release or two).

RBTools has historically been less actively developed than Review Board (it's primarily been 2 people total for the majority of all our development), but we have a dedicated maintainer now for RBTools.

atagar March 7, 2013, 7:10 p.m.

> There will be a 0.5 release out soon that drastically changes the codebase.

Neat, glad to hear it! Any guess roughly when it'll be released? Re-applying our patch set to 0.4 has been slowly crawling up my todo list but if 0.5 is just around the corner then I might as well wait for that (and update these code reviews with the rebased versions).

chipx86 March 8, 2013, 1:32 a.m.

We're working to get the documentation in shape now, and fix any bugs that crop up, so it's pretty close.

Change Summary:

Bump? Re-applied against the release-0.5 tag and did a simple sanity test that posting a git backed code review worked...

https://github.com/atagar/rbtools/tree/timeout_retries
https://github.com/atagar/rbtools/commit/db13e513137957ccc75a00e7b39e4591cad0b4ce

Diff:

Revision 2 (+22 -15)

Show changes

rbtools/postreview.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/postreview.py
  Ignored Files:

rbtools/postreview.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (87 > 79 characters)
```
rbtools/postreview.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (108 > 79 characters)
```
rbtools/postreview.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (100 > 79 characters)
```
rbtools/postreview.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (95 > 79 characters)
```

Change Summary:

Gah I hate E501. Oh well, when in rome...

Diff:

Revision 3 (+28 -15)

Show changes

rbtools/postreview.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/postreview.py
  Ignored Files:

Change Summary:

Oops, forgot a couple other fixes we've collected for this retry handling...

* Not all URLError instances have a reason attribute.
* Callers may legitamately want to receive HTTPError instances (this accidentally caught them since they're a URLError subclass).

You could repro the issue caused by the later by doing...

1. 'postreview.py CLN' (this will complete successfully)
2. 'postreview.py CLN' again (this will fail with a 409 rather than update the prior one)

Diff:

Revision 4 (+34 -15)

Show changes

rbtools/postreview.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/postreview.py
  Ignored Files:

rbtools/postreview.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 16
 E111 indentation is not a multiple of four
```
rbtools/postreview.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Col: 16
 E113 unexpected indentation
```

Change Summary:

Minor indent tweak.

Diff:

Revision 5 (+34 -15)

Show changes

rbtools/postreview.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/postreview.py
  Ignored Files:

rbtools/postreview.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 18
 E111 indentation is not a multiple of four
```
rbtools/postreview.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 18
 E113 unexpected indentation
```
rbtools/postreview.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 17
 E901 IndentationError
```

Change Summary:

Ok, I fail at counting.

Diff:

Revision 6 (+34 -15)

Show changes

rbtools/postreview.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/postreview.py
  Ignored Files:

rbtools/postreview.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 17
 E113 unexpected indentation
```
rbtools/postreview.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Col: 17
 E112 expected an indented block
```

Change Summary:

Fine, fine, installed pep8 just to check this damn thing. It shows a few issues...

moirai:~% pep8 postreview.py 
postreview.py:38:80: E501 line too long (82 characters)
postreview.py:60:24: E261 at least two spaces before inline comment
postreview.py:153:20: E221 multiple spaces before operator
postreview.py:970:5: E303 too many blank lines (2)
postreview.py:1370:27: E201 whitespace after '('

But that could be accounted for via ignores or a version difference. This fixes what this instance is swawking about, anyway.

Diff:

Revision 7 (+34 -15)

Show changes

rbtools/postreview.py

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/postreview.py
  Ignored Files:

Fix it!

```
Sorry for the long delay on this.
```
rbtools/postreview.py (Diff revision 7)
An issue was opened. Show all issues
```
Please add a docstring for this method.
```

rbtools/postreview.py (Diff revision 7)

An issue was opened. Show all issues

Instead of using the continuation character \, please wrap the condition in parentheses.

rbtools/postreview.py (Diff revision 7)

An issue was opened. Show all issues

Why the sleep?

This should also probably have a handler for the case where tries == 0, and show an error that it tried and gave up.

Status:: Discarded