You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "danielcweeks (via GitHub)" <gi...@apache.org> on 2023/12/01 23:09:08 UTC
[PR] Core: REST HttpClient connections config [iceberg]
danielcweeks opened a new pull request, #9195:
URL: https://github.com/apache/iceberg/pull/9195
The default config for http connections is only 25 connections and 5 per host. Since the REST client is typically talking to a single host, the number of connections allowed is relatively small and can cause contention where the client is shared across multiple running jobs (Spark/Trino/Kafka Connect, etc.).
This PR increases the defaults and allows for configuring the values. The pool grows dynamically as connections are needed and reused if available, so higher defaults should not impact typical usage, but allows for growth if there is contention.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: REST HttpClient connections config [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9195:
URL: https://github.com/apache/iceberg/pull/9195#discussion_r1412727587
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -72,6 +74,10 @@ public class HTTPClient implements RESTClient {
static final String CLIENT_GIT_COMMIT_SHORT_HEADER = "X-Client-Git-Commit-Short";
private static final String REST_MAX_RETRIES = "rest.client.max-retries";
+ private static final String REST_MAX_CONNECTIONS = "rest.client.max-connections";
+ private static final int REST_MAX_CONNECTIONS_DEFAULT = 100;
+ private static final String REST_MAX_CONNECTIONS_PER_ROUTE = "rest.client.connections-per-route";
+ private static final int REST_MAX_CONNECTIONS_PER_ROUTE_DEFAULT = 100;
Review Comment:
Nit: I guess you could just assign REST_MAX_CONNECTIONS_PER_ROUTE_DEFAULT to REST_MAX_CONNECTIONS_DEFAULT since the default will probably always be 1:1 with the per route default/ but no big deal since I don't expect the defaults to change anyways
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: REST HttpClient connections config [iceberg]
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #9195:
URL: https://github.com/apache/iceberg/pull/9195#discussion_r1412727587
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -72,6 +74,10 @@ public class HTTPClient implements RESTClient {
static final String CLIENT_GIT_COMMIT_SHORT_HEADER = "X-Client-Git-Commit-Short";
private static final String REST_MAX_RETRIES = "rest.client.max-retries";
+ private static final String REST_MAX_CONNECTIONS = "rest.client.max-connections";
+ private static final int REST_MAX_CONNECTIONS_DEFAULT = 100;
+ private static final String REST_MAX_CONNECTIONS_PER_ROUTE = "rest.client.connections-per-route";
+ private static final int REST_MAX_CONNECTIONS_PER_ROUTE_DEFAULT = 100;
Review Comment:
Nit: I guess REST_MAX_CONNECTIONS_PER_ROUTE_DEFAULT could be assigned to REST_MAX_CONNECTIONS_DEFAULT since the default will probably always be 1:1 with the per route default/ but no big deal since I don't expect the defaults to change anyways
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: REST HttpClient connections config [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #9195:
URL: https://github.com/apache/iceberg/pull/9195#discussion_r1412877063
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -72,6 +74,10 @@ public class HTTPClient implements RESTClient {
static final String CLIENT_GIT_COMMIT_SHORT_HEADER = "X-Client-Git-Commit-Short";
private static final String REST_MAX_RETRIES = "rest.client.max-retries";
+ private static final String REST_MAX_CONNECTIONS = "rest.client.max-connections";
+ private static final int REST_MAX_CONNECTIONS_DEFAULT = 100;
+ private static final String REST_MAX_CONNECTIONS_PER_ROUTE = "rest.client.connections-per-route";
+ private static final int REST_MAX_CONNECTIONS_PER_ROUTE_DEFAULT = 100;
Review Comment:
That's true, but it might be a little confusing so kept separate defaults.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Core: REST HttpClient connections config [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks merged PR #9195:
URL: https://github.com/apache/iceberg/pull/9195
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org