Add retry support to RequestsContext.

Review Request #1303 — Created Nov. 7, 2014 and submitted

jcohen
pex
49872f6...
pants-reviews
ity, jsirois, stuhood, wickman

commit dfdf21ce5c9aa2db8259f0579519ad5bbbe17ed5
Author: Joshua Cohen jcohen@twitter.com
Date: Fri Nov 7 15:11:25 2014 -0800

Add retry support to RequestsContext.

pex/http.py | 35 +++++++++++++++----
tests/test_http.py | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 127 insertions(+), 8 deletions(-)

$ tox

  • 0
  • 0
  • 1
  • 2
  • 3
Description From Last Updated
JC
  1. This fixes https://github.com/pantsbuild/pex/issues/18

  2. 
      
IT
  1. thanks for adding the tests, almost there!

  2. pex/http.py (Diff revision 1)
     
     

    how about moving this to a flag so we can set a default?

    1. added an environment variable so we can override this regardless of how pex is being invoked (presumably pants doesn't shell out to the pex command line, but consumes it as a lib, in which case a flag in pex won't do us any good).

  3. pex/http.py (Diff revision 1)
     
     

    abstract out to create_session(..) or something similar

  4. pex/http.py (Diff revision 1)
     
     

    this is using tabs instead of spaces

    1. I don't think so...

      $ grep $'\t' pex/http.py  |wc -l
         0
      

      maybe a reviewboard glitch?

  5. pex/http.py (Diff revision 1)
     
     

    use .format instead

    1. Every other usage of TRACER in pex follows this pattern. I'm going to leave it as is?

  6. 
      
JC
JC
  1. ping

  2. 
      
LA
  1. 
      
  2. pex/http.py (Diff revision 2)
     
     

    Did you try this? I'd think you need to convert the env var to an int

    $ export TWO=2
    [tw-172-25-20-104 lahosken_pants (intellij_plugin)]$ python
    Python 2.7.8 (default, Oct 20 2014, 11:21:04) 
    [GCC 4.2.1 Compatible Apple LLVM 5.0 (clang-500.2.79)] on darwin
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import os
    >>> two = os.environ.get('TWO', 7)
    >>> two
    '2'
    
    1. Good catch, fixed!

  3. 
      
JC
IT
  1. Ship It!

  2. 
      
JC
  1. ping wickman

  2. 
      
YA
  1. One question, but not a ship blocker. Thanks for tackling!

  2. pex/http.py (Diff revision 3)
     
     

    Any thoughts on if we should do anything with timeouts[1] here?

    [1] http://docs.python-requests.org/en/latest/user/advanced/#timeouts

    1. That's a good question. It looks like pip uses a default timeout of 15 seconds and allows it to be overridden on the command line or via environment variables. Would it make sense for us to adopt a similar behavior?

    2. Discussed w/ Brian. Timeouts are orthoganal to this change, filed https://github.com/pantsbuild/pex/issues/26 to track a future enhancement.

  3. tests/test_http.py (Diff revision 3)
     
     

    whoa, nicely done

  4. 
      
WI
  1. 
      
  2. pex/http.py (Diff revision 3)
     
     
     

    does:

    except requests.exceptions.ReadTimeout as e:
    TRACER.log(...)
    except requests.exceptions.RequestException as e:
    raise self.Error(e)

    do the same thing?

    1. I had it that way at one point, and switched it to this version for some reason. Thinking back it may have been related to a bug that I ended up fixing. Let me take a look and see if there's any reason not to do it this way.

  3. 
      
JC
JC
JS
  1. Looks like this was submitted here: https://github.com/pantsbuild/pex/commit/c0174529e49acabbe631556c85e04ea2735ab085
    Please mark this review submitted.

    1. Joshua is out on vacation and I get HTTP 403. Are you able to close on his behalf?

    2. Nope.

    3. Done!

  2. 
      
JC
Review request changed

Status: Closed (submitted)

Loading...