Skip to content

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

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.

Sign up for GitHub

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

Closed
kushagraThapar wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0cf93fb
Added ability to pass connection policy details to http transport cli…
kushagraThapar Jan 4, 2020
d85df09
Merge branch 'master' into fix_http_transport_client_connection_config
kushagraThapar Jan 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions sdk/cosmos/microsoft-azure-cosmos/src/main/java/com/azure/data/cosmos/internal/Configs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ public class Configs {
// Reactor Netty Constants
private static final int MAX_IDLE_CONNECTION_TIMEOUT_IN_MILLIS = 60 * 1000;
private static final int CONNECTION_ACQUIRE_TIMEOUT_IN_MILLIS = 45 * 1000;
private static final int REACTOR_NETTY_MAX_CONNECTION_POOL_SIZE = 1000;
private static final String REACTOR_NETTY_CONNECTION_POOL_NAME = "reactor-netty-connection-pool";

public Configs() {
Expand Down Expand Up @@ -159,10 +158,6 @@ public int getConnectionAcquireTimeoutInMillis() {
return CONNECTION_ACQUIRE_TIMEOUT_IN_MILLIS;
}

public int getReactorNettyMaxConnectionPoolSize() {
return REACTOR_NETTY_MAX_CONNECTION_POOL_SIZE;
}

private static String getJVMConfigAsString(String propName, String defaultValue) {
String propValue = System.getProperty(propName);
return StringUtils.defaultString(propValue, defaultValue);
Expand Down
2 changes: 1 addition & 1 deletion ...osoft-azure-cosmos/src/main/java/com/azure/data/cosmos/internal/RxDocumentClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ private void initializeDirectConnectivity() {

this.storeClientFactory = new StoreClientFactory(
this.configs,
this.connectionPolicy.requestTimeoutInMillis() / 1000,
this.connectionPolicy,
// this.maxConcurrentConnectionOpenRequests,
0,
this.userAgentContainer
Expand Down
15 changes: 9 additions & 6 deletions .../src/main/java/com/azure/data/cosmos/internal/directconnectivity/HttpTransportClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import com.azure.data.cosmos.BadRequestException;
import com.azure.data.cosmos.BridgeInternal;
import com.azure.data.cosmos.ConflictException;
import com.azure.data.cosmos.ConnectionPolicy;
import com.azure.data.cosmos.CosmosClientException;
import com.azure.data.cosmos.ForbiddenException;
import com.azure.data.cosmos.GoneException;
Expand Down Expand Up @@ -67,18 +68,20 @@ public class HttpTransportClient extends TransportClient {
private final Map<String, String> defaultHeaders;
private final Configs configs;

HttpClient createHttpClient(int requestTimeout) {
HttpClient createHttpClient(ConnectionPolicy connectionPolicy) {
// TODO: use one instance of SSL context everywhere
HttpClientConfig httpClientConfig = new HttpClientConfig(this.configs);
httpClientConfig.withRequestTimeoutInMillis(requestTimeout * 1000);
httpClientConfig.withPoolSize(configs.getDirectHttpsMaxConnectionLimit());
HttpClientConfig httpClientConfig = new HttpClientConfig(this.configs)
.withMaxIdleConnectionTimeoutInMillis(connectionPolicy.idleConnectionTimeoutInMillis())
.withPoolSize(connectionPolicy.maxPoolSize())
.withHttpProxy(connectionPolicy.proxy())
.withRequestTimeoutInMillis(connectionPolicy.requestTimeoutInMillis());

return HttpClient.createFixed(httpClientConfig);
}

public HttpTransportClient(Configs configs, int requestTimeout, UserAgentContainer userAgent) {
public HttpTransportClient(Configs configs, ConnectionPolicy connectionPolicy, UserAgentContainer userAgent) {
this.configs = configs;
this.httpClient = createHttpClient(requestTimeout);
this.httpClient = createHttpClient(connectionPolicy);

this.defaultHeaders = new HashMap<>();

Expand Down
7 changes: 4 additions & 3 deletions ...s/src/main/java/com/azure/data/cosmos/internal/directconnectivity/StoreClientFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

package com.azure.data.cosmos.internal.directconnectivity;

import com.azure.data.cosmos.ConnectionPolicy;
import com.azure.data.cosmos.internal.Configs;
import com.azure.data.cosmos.internal.IAuthorizationTokenProvider;
import com.azure.data.cosmos.internal.SessionContainer;
Expand All @@ -22,17 +23,17 @@ public class StoreClientFactory implements AutoCloseable {

public StoreClientFactory(
Configs configs,
int requestTimeoutInSeconds,
ConnectionPolicy connectionPolicy,
int maxConcurrentConnectionOpenRequests,
UserAgentContainer userAgent) {

this.configs = configs;
this.protocol = configs.getProtocol();
this.requestTimeoutInSeconds = requestTimeoutInSeconds;
this.requestTimeoutInSeconds = connectionPolicy.requestTimeoutInMillis() / 1000;
this.maxConcurrentConnectionOpenRequests = maxConcurrentConnectionOpenRequests;

if (protocol == Protocol.HTTPS) {
this.transportClient = new HttpTransportClient(configs, requestTimeoutInSeconds, userAgent);
this.transportClient = new HttpTransportClient(configs, connectionPolicy, userAgent);
} else if (protocol == Protocol.TCP){
this.transportClient = new RntbdTransportClient(configs, requestTimeoutInSeconds, userAgent);
} else {
Expand Down
2 changes: 1 addition & 1 deletion .../microsoft-azure-cosmos/src/main/java/com/azure/data/cosmos/internal/http/HttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ static HttpClient createFixed(HttpClientConfig httpClientConfig) {
}

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

@moderakh moderakh Jan 7, 2020

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?

@kushagraThapar kushagraThapar Jan 7, 2020

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

@moderakh moderakh Jan 7, 2020

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.

@kushagraThapar kushagraThapar Jan 7, 2020

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.

if (httpClientConfig.getMaxPoolSize() != null) {
maxPoolSize = httpClientConfig.getMaxPoolSize();
}
Expand Down
5 changes: 3 additions & 2 deletions .../test/java/com/azure/data/cosmos/internal/directconnectivity/HttpTransportClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import com.azure.data.cosmos.BadRequestException;
import com.azure.data.cosmos.ConflictException;
import com.azure.data.cosmos.ConnectionPolicy;
import com.azure.data.cosmos.ForbiddenException;
import com.azure.data.cosmos.GoneException;
import com.azure.data.cosmos.InternalServerErrorException;
Expand Down Expand Up @@ -109,11 +110,11 @@ public static HttpTransportClient getHttpTransportClientUnderTest(int requestTim
HttpClient httpClient) {
class HttpTransportClientUnderTest extends HttpTransportClient {
public HttpTransportClientUnderTest(int requestTimeout, UserAgentContainer userAgent) {
super(configs, requestTimeout, userAgent);
super(configs, ConnectionPolicy.defaultPolicy().requestTimeoutInMillis(requestTimeout * 1000), userAgent);
}

@Override
HttpClient createHttpClient(int requestTimeout) {
HttpClient createHttpClient(ConnectionPolicy connectionPolicy) {
return httpClient;
}
}
Expand Down