[PATCH] D38281: [llvmlab] Add timeout and retries for fetching builds.

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: llvmbisect/llvmlab/gcs.py:15
 
+class HttpClient(object):
+    def __init__(self):
----------------
vsapsai wrote:
> JDevlieghere wrote:
> > Maybe we should call this `HttpSession` to emphasize that we will need a new instance each time?
> In this code it doesn't really matter if you create a new instance or reuse the old one. In most places I decided to create a new instance because it is simpler.
> 
> I've chosen name `HttpClient` because `client.get(...)` seems more natural compared to `session.get(...)`. I have to acknowledge my preferences are influenced by Java (e.g. org.apache.http.impl.client.CloseableHttpClient, okhttp3.OkHttpClient). Though looks like in Python "client" is also popular according to renaming `httplib` to `http.client`. 
Fair enough, if it doesn't matter I'm totally fine with naming it client! 


================
Comment at: llvmbisect/llvmlab/gcs.py:53
              'prefix': project + "/"}
-    r = requests.get(GCS + "b/" + BUCKET + "/o", params=params)
+    http = HttpClient()
+    r = http.get(GCS + "b/" + BUCKET + "/o", params=params)
----------------
Any argument against moving this up and having all the functions reusing this single instance? 


https://reviews.llvm.org/D38281