- Notifications
You must be signed in to change notification settings - Fork 48
Capability to configure timeouts #43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
kylekampy left a comment
There was a problem hiding this 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.
There was a problem hiding this 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.
| There are two approaches. |
Yes, let's do that and there is no need to make them do any additional steps via |
kylekampy left a comment
There was a problem hiding this 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.
NotificationHubs/src/com/windowsazure/messaging/HttpClientManager.java Outdated Show resolved Hide resolved
NotificationHubs/src/com/windowsazure/messaging/HttpClientManager.java Outdated Show resolved Hide resolved
NotificationHubs/src/com/windowsazure/messaging/HttpClientManager.java Outdated Show resolved Hide resolved
NotificationHubs/src/com/windowsazure/messaging/HttpClientManager.java Outdated Show resolved Hide resolved
mpodwysocki left a comment
There was a problem hiding this 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
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()