-
Notifications
You must be signed in to change notification settings - Fork 60
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
Improving http request time out avoiding region failover for high latency request #360
Improving http request time out avoiding region failover for high latency request #360
Conversation
sdk/src/test/java/com/microsoft/azure/cosmosdb/rx/internal/HttpRequestTimeoutTest.java
Outdated
Show resolved
Hide resolved
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 fix the minor comments. other than that LGTM.
sdk/src/test/java/com/microsoft/azure/cosmosdb/rx/internal/HttpRequestTimeoutTest.java
Outdated
Show resolved
Hide resolved
sdk/src/test/java/com/microsoft/azure/cosmosdb/rx/internal/HttpRequestTimeoutTest.java
Outdated
Show resolved
Hide resolved
sdk/src/test/java/com/microsoft/azure/cosmosdb/rx/internal/HttpRequestTimeoutTest.java
Outdated
Show resolved
Hide resolved
...microsoft/azure/cosmosdb/rx/internal/directconnectivity/ClientSideRequestStatisticsTest.java
Outdated
Show resolved
Hide resolved
commons/src/main/java/com/microsoft/azure/cosmosdb/ClientSideRequestStatistics.java
Show resolved
Hide resolved
sdk/src/test/java/com/microsoft/azure/cosmosdb/rx/internal/HttpRequestTimeoutTest.java
Outdated
Show resolved
Hide resolved
commons/src/main/java/com/microsoft/azure/cosmosdb/ClientSideRequestStatistics.java
Show resolved
Hide resolved
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.
LGTM
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.
Left minor comment regarding license. Please take a look. Thanks for including the QueryPlan change.
Currently we have an api on ConnectionPolicy setRequestTimeoutInMillis which sets the request time out for both TCP and HTTP requests.
We want to cap the lower value of http client request time out to 60 sec, as http network request call ends up into region fail over and also Gateway does not have any sla.
Note: This PR also contains the improvement on AddressResolutionStatistics in ClientSideRequestStatistics, which include error message if there is any failure on address cache refresh call, also including flag inflightRequest which will indicate whether address call finished in lifetime of the the request.
Also closes query plan issue
closes #359