Make pinger.py work with both HTTP and HTTPS.

Review Request #3904 — Created May 17, 2016 and submitted

gmalmquist
pants
gmalmquist/pinger-https
3463
pants-reviews
benjyw, patricklaw, stuhood, zundel

Our remote artifact cache serves over HTTPS, which the RESTful
artifact cache ostensibly supports, but the Pinger was previously
checking what URLs are reachable using only HTTP.

This change switches pinger to use requests instead of httplib,
which gives us HTTPS support "for-free".

Added test to pinger.py. All tests in pinger.py pass locally.

Jenkins went green: http://jenkins.pantsbuild.org/job/pantsbuild/job/pants/branch/PR-3463/5/

  • 0
  • 0
  • 3
  • 0
  • 3
Description From Last Updated
BE
  1. 
      
  2. src/python/pants/cache/pinger.py (Diff revision 1)
     
     
    If you're feeling ambitious, we could also replace this with 
    
    ```
    @classmethod
    @memoized_method
    def _get_ping_time(url_info]
    
    ```
    
    On the Pinger class, instead of manually implementing the memoization.
    
    Up to you though.
    1. Wouldn't memoized method just record whatever the result of the first invocation? I'm not sure that true memoization is what we want here. We want this value to change when the URL goes from unreachable to reachable if someone ever calls ping() more than once (which it does by design)

    2. This method itself is what calls ping() multiple times, so that's not a problem. And it memoizes with the key (url, timeout, tries), not just the url.

  3. src/python/pants/cache/pinger.py (Diff revision 1)
     
     

    Maybe it's worth going the distance and using requests instead of urllib2? We already use requests in several places, and urllib2 only in one other, obscure place.

    The two lines collapse down to: requests.head(url, timeout=timeout) which is a little neater, and is also explicit about the fact that it uses HEAD, instead of hiding that behind the somewhat opaque info() call.

    1. I'm for this, I asked @gmalmquist to switch to urllib from httplib for similar reasons (httplib requires a different method call to do https) but I didn't think about moving to requests. Plus, fixing up frustrating cert errors all work the same way through the requests lib.

    2. sounds good

  4. Why do we need this? Does urllib2's conn.info() send a GET request, not a HEAD request?

    If so, I'm even more in favor of using requests instead of urllib2.

  5. 
      
ZU
  1. 
      
  2. nit: they aren't xfailed. @pytest.xfail means that it runs the test, but in order to pass CI the test must fail. If the problem is ever fixed, the test will pass and pytest will cause the test run to fail. https://pytest.org/latest/skipping.html

  3. thanks for your efforts to write a better test

  4. 
      
GM
GM
BE
  1. Thanks for the changes. Looks great!

  2. src/python/pants/cache/cache_setup.py (Diff revision 2)
     
     

    Dict comprehensions, nice!

    1. I'm going to assume you didn't mean to enable the "open an issue" option for this comment x-)

    2. Ooops, it's not my day...

  3. 
      
GM
ZU
  1. Ship It!
  2. 
      
GM
Review request changed

Status: Closed (submitted)

Change Summary:

In e662399b8c6810deee53f16556c7d5cc1cfe37c8, thanks Benjy & Eric!

Loading...