ApacheCloudStackClient: Allows connection timeout to be specified#27
Conversation
|
Can one of the admins verify this patch? |
rafaelweingartner
left a comment
rafaelweingartner
left a comment
There was a problem hiding this comment.
@rhtyd I think the change could benefit from a little improvement. Please, check my comments and let me know what you think.
| */ | ||
| protected CloseableHttpClient createHttpClient() { | ||
| HttpClientBuilder httpClientBuilder = HttpClientBuilder.create(); | ||
| RequestConfig config = RequestConfig.custom() |
There was a problem hiding this comment.
Instead of "1000", what about using the constant DateUtils.MILLIS_PER_SECOND?
Also, I think it is a good idea to extract lines 199-202, this enables docs (for instance explaining these three different timeout configurations that the method creates) and a unit test case.
Also, do not forget to add the verification on “br.com.autonomiccs.apacheCloudStack.client.ApacheCloudStackClientTest.configureExecuteAndVerifyTestForCreateHttpClient(boolean, int)” for a single method call for the newly created method that creates “RequestConfig” with connection timeouts.
|
add to whitelist |
|
@rafaelweingartner thanks for the review, I've updated the PR per your suggestions. Please re-review. |
rafaelweingartner
left a comment
rafaelweingartner
left a comment
There was a problem hiding this comment.
Thanks for the improvements, but I still thinks that "createRequestConfig" method needs a unit test
| * This can be used to set on a HttpClient its connect timeout, connection request timeout and socket timeout. | ||
| * @return | ||
| */ | ||
| protected RequestConfig createRequestConfig() { |
There was a problem hiding this comment.
There is a typo in the Java doc, "It createsa", I believe it is "It creates a".
Also, I think you can remove the "@return" as it is not being used there.
Could you also create a test case for this method? In my opinion, there is the need for a test to check if the connectionTimeout is, in fact, the timeout value configured by this method.
There was a problem hiding this comment.
Thanks @rafaelweingartner I've incorporated your feedback, please re-review the changes now.
|
@rafaelweingartner thanks, changes fixed now. Please re-review. |
rafaelweingartner
left a comment
rafaelweingartner
left a comment
There was a problem hiding this comment.
Nice job @rhtyd,
I only have a final remark in the test cases you added.
| Mockito.verify(httpClientMock).close(); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Test cases are always awesome, but I will be picky one more time.
Lines 140 – 143 and 150-153 are basically the same, the only difference is the timeout value.
What about extracting them for a single method that receives the expected timeout value as a parameter?
There was a problem hiding this comment.
Okay, I'll refactor this tomorrow. It's late for me (1am here).
|
@rafaelweingartner alright fixed the unit test too, please accept it and help create the 1.0.4 release with the changes. Thanks. |
|
@rhtyd please squash the commits |
- Allows a custom connection timeout (in seconds) to be specified - Refactors code to createRequestConfig() with javadoc and adds/updates a unit test - Uses DateUtils.MILLIS_PER_SECOND - Uses a default connection timeout of 60 seconds Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
yadvr
force-pushed
the
connection-timeout
branch
from
204d2c9 to
eb72a9c
Compare
|
@rafaelweingartner done (ps. there is a way to squash and merge the PR in the Github UI?) |
|
@rhtyd not that I know of. |
|
Thanks @rafaelweingartner |
|
Aha, I never clicked on this arrow before. I would always clicked on the green and shine button and get the merge commit box. BTW: I just merged and closed the version. There is a delay of 30-60 min. until the jar gets into the maven central repo. |
|
Thanks @rafaelweingartner looking forward to using the 1.0.4 release! |
3 participants
Ping @rafaelweingartner for review