Adding retry to getHttpClient when "No System TLS" pops up #491 #597
Conversation
Codecov Report
@@ Coverage Diff @@
## master #597 +/- ##
============================================
- Coverage 59.73% 59.42% -0.32%
Complexity 470 470
============================================
Files 36 36
Lines 3050 3066 +16
Branches 255 258 +3
============================================
Hits 1822 1822
- Misses 1086 1102 +16
Partials 142 142 |
| } | ||
| } | ||
| } | ||
| LOG.warn(String.format("Last attempt to get the HttpClient for url %s", url)); |
There was a problem hiding this comment.
Will this log every time a client is requested? It looks this way?
There was a problem hiding this comment.
Oh my bad. There is a return statement there. I read it too quickly.
There was a problem hiding this comment.
I’m not sure if that will log at all will it? Since it’s after the throw and outside the for loop
There was a problem hiding this comment.
Yes, that is dead code. I'll remove it, I need to leave the return though.
| LOG.debug(message, e); | ||
| if (i == 2) { | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
So, you don't think there should be a random sleep? Just retrying it straight away seems to fix it in all cases?
There was a problem hiding this comment.
It worked right away in all cases I tried. But I agree with you, it is a good idea to add a small delay, random between 1 to 5 seconds.
| } catch (Exception | AssertionError e) { | ||
| String message = String.format("Error while getting the HttpClient for url %s, attempt #%s", url, (i+1)); | ||
| LOG.debug(message, e); | ||
| if (i == 2) { |
There was a problem hiding this comment.
This should have some relation to maxTries. If you started with i=1 then you could use i == maxTries instead
| public HttpClient getHttpClient(URL url) { | ||
| // https://github.com/zalando/zalenium/issues/491 | ||
| int maxTries = 3; | ||
| for (int i = 0; i < maxTries; i++) { |
There was a problem hiding this comment.
Maybe for (int i = 1; i <= maxTries; i++) might work better here, so you can handle the exception throw easier.
There was a problem hiding this comment.
Right that is more simple.
| } | ||
| } | ||
| LOG.warn(String.format("Last attempt to get the HttpClient for url %s", url)); | ||
| return httpClientFactory.createClient(url); |
There was a problem hiding this comment.
Do we actually need this return at all? I think it might be dead code. Because when it fails it throws an exception.
The compiler might not be able to figure out it’s dead code. So maybe you could just throw an IllegalStateException. Since I think it’s not even possible to get there.
|
👍 |
1 similar comment
|
👍 |
To solve #491