Skip to content

Conversation

@Alessar
Copy link
Member

@Alessar Alessar commented Mar 6, 2019

Before first using HttpClientManager.getHttpAsyncClient() there is capability to set operation timeouts.

The timeout in milliseconds used when requesting a connection from the connection manager.
HttpClientManager.setConnectionRequestTimeout()

The timeout in milliseconds until a connection is established.
HttpClientManager.setConnectTimeout()

The socket timeout in milliseconds, which is the timeout for waiting for data or,
put differently, a maximum period inactivity between two consecutive data packets.
HttpClientManager.setSocketTimeout()

@Alessar Alessar requested a review from stankovski March 6, 2019 14:28
Copy link

@kylekampy kylekampy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but I would wait for @stankovski or @brannon to take a look before merging anything.

@itoys itoys self-requested a review March 6, 2019 14:57
Copy link
Contributor

@itoys itoys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that we do not need those changes. If someone need to setup timeouts they can use setHttpAsyncClient method. Also this implementation is confused, seems like we can change timeouts in anytime, but it isn't true.

@Alessar Alessar requested a review from itoys March 6, 2019 15:11
@Alessar
Copy link
Member Author

Alessar commented Mar 7, 2019

There are two approaches.
One way is to force user to do a lot of additional steps via setHttpAsyncClient.
Second way is to allow user to do some small changes via setting timeouts.
I prefer second way.

@Alessar Alessar requested a review from brannon March 7, 2019 10:18
@mpodwysocki
Copy link
Contributor

There are two approaches.
One way is to force user to do a lot of additional steps via setHttpAsyncClient.
Second way is to allow user to do some small changes via setting timeouts.
I prefer second way.

Yes, let's do that and there is no need to make them do any additional steps via setHttpAsyncClient.

@Alessar Alessar requested a review from mpodwysocki March 15, 2019 07:16
Copy link

@kylekampy kylekampy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small suggestions in my review this time around.

I tried to imagine I'm the end user figuring out this SDK for the first time and thought about where I might get confused around usage. The main concern is around get initializing a default client, but I think that can be ok as long as we help give the user hints about what they are doing wrong when we throw exceptions at them.

Overall though I think it provides the right amount of flexibility to allow overriding the timeouts.

Copy link
Contributor

@mpodwysocki mpodwysocki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the following to naming conventions, more comments and add some JavaDocs

@stankovski stankovski merged commit f855a18 into master Mar 20, 2019
@Alessar Alessar deleted the timeout branch March 20, 2019 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

7 participants