Skip to content

Ability to pass connection policy configuration values to http client in transport layer#7126

Closed
kushagraThapar wants to merge 2 commits into
Azure:masterfrom
kushagraThapar:fix_http_transport_client_connection_config
Closed

Ability to pass connection policy configuration values to http client in transport layer#7126
kushagraThapar wants to merge 2 commits into
Azure:masterfrom
kushagraThapar:fix_http_transport_client_connection_config

Conversation

@kushagraThapar

@kushagraThapar kushagraThapar commented Jan 4, 2020

Copy link
Copy Markdown
Member

Currently end users cannot configure http client created in http transport client layer. Connection Policy configuration fields like idleConnectionTimeout, max pool size, and http proxy do not get passed to the http client created in HttpTransportClient.

  • This PR fixes that issue by passing connectionPolicy object to HttpTransportClient so as to be able to configure the http client by these configurations.
  • Walmart is one example, where they want to set a higher value for idle connection timeout.

@kushagraThapar kushagraThapar changed the title Added ability to pass connection policy details to http transport client Ability to pass connection policy configuration values to http client in transport layer Jan 4, 2020
@kushagraThapar

Copy link
Copy Markdown
Member Author

/azp run java - cosmos - tests

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@kushagraThapar

Copy link
Copy Markdown
Member Author

/azp run java - cosmos - tests

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).


// Default pool size
Integer maxPoolSize = httpClientConfig.getConfigs().getReactorNettyMaxConnectionPoolSize();
Integer maxPoolSize = httpClientConfig.getConfigs().getDirectHttpsMaxConnectionLimit();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isn't HttpClient.createFixed(.) also used for GW?
How does HttpClient.createFixed(.) distinguish between GW and DirectHttps?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, it is used for both Gateway and Direct Https. They are different and I think that is the problem.
There is no way to set idle connection timeout, max pool size, etc. for Direct Https Client, because it doesn't have connectionPolicy object .
While Gateway Https client reads these values from connectionPolicy object which gets passed to it when we create https client.

To solve this problem, in this PR, I have kept same configuration for Direct Https and Gateway Http Clients. So that end users can configure these http clients through connectionPolicy object.

I believe this problem also persists in V2.
You can see both Gateway and Direct Http clients are built using different configuration.
Gateway: https://github.com/Azure/azure-cosmosdb-java/blob/master/sdk/src/main/java/com/microsoft/azure/cosmosdb/rx/internal/RxDocumentClientImpl.java#L392
Direct Https: https://github.com/Azure/azure-cosmosdb-java/blob/master/direct-impl/src/main/java/com/microsoft/azure/cosmosdb/internal/directconnectivity/HttpTransportClient.java#L81

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@kushagraThapar that is kind of on purpose.

GW and Direct are two different modes, and even in Direct mode you may need different configuration setting for the connections to BE nodes vs connections to GW (for query).

As an example, RNTBD has its own way of configuration for its connection stack.
I think we might need ConnectionPolicy#idleTimeToGW and ConnectionPolicy#idleTimeForDiredct something along on those lines. similar for connection pool size.

Let's say if you have 4000 replicas your connection pool size for direct at least has to be >= 4000. but as we know even in Direct mode some request go to GW we need a separate connection pool to GW. With the change you made, increasing Direct connection pool size will unnecessarily increase the connection pool size for GW as well which is not the right thing to do.

Ping me to discuss this offline over teams.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agreed, let's talk offline.

@kushagraThapar

Copy link
Copy Markdown
Member Author

I looked at the CI failure logs.
ReadMyWrites failed because of Conflict exception (resource already exists). This is the same issue we fixed on v2 (don't retry on writes). .
I have ported the fix to both v3 and v4.
Let's merge those ports first, then we can run the CI on this PR again.

Btw, here is the port for V3: #7240

@joshfree joshfree added Client This issue points to a problem in the data-plane of the library. Cosmos labels Jul 9, 2020
@joshfree joshfree added this to the Backlog milestone Jul 9, 2020
@joshfree

joshfree commented Sep 1, 2020

Copy link
Copy Markdown
Member

@kushagraThapar can this 9-month old PR be closed out?

@kushagraThapar

Copy link
Copy Markdown
Member Author

@kushagraThapar can this 9-month old PR be closed out?

lol, yeah let me close it :)

@kushagraThapar

Copy link
Copy Markdown
Member Author

This is done using GatewayConnectionConfig and DirectConnectionConfig. No need for this PR anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Client This issue points to a problem in the data-plane of the library. Cosmos

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants