You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2023/08/22 06:01:31 UTC
[GitHub] [iceberg] nastra opened a new pull request, #8366: Core: Add exponential retry strategy to REST client
nastra opened a new pull request, #8366:
URL: https://github.com/apache/iceberg/pull/8366
This adds a new retry strategy, allowing exponential backoff and also allows to adjust the number of retries for the `DefaultHttpRequestRetryStrategy`
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #8366: Core: Add exponential retry strategy to REST client
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #8366:
URL: https://github.com/apache/iceberg/pull/8366#discussion_r1301820236
##########
core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.ConnectException;
+import java.net.NoRouteToHostException;
+import java.net.UnknownHostException;
+import java.time.Instant;
+import java.util.Set;
+import java.util.concurrent.ThreadLocalRandom;
+import javax.net.ssl.SSLException;
+import org.apache.hc.client5.http.HttpRequestRetryStrategy;
+import org.apache.hc.client5.http.utils.DateUtils;
+import org.apache.hc.core5.concurrent.CancellableDependency;
+import org.apache.hc.core5.http.ConnectionClosedException;
+import org.apache.hc.core5.http.Header;
+import org.apache.hc.core5.http.HttpHeaders;
+import org.apache.hc.core5.http.HttpRequest;
+import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.Method;
+import org.apache.hc.core5.http.protocol.HttpContext;
+import org.apache.hc.core5.util.TimeValue;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+
+/**
+ * Defines an exponential HTTP request retry strategy and provides the same characteristics as the
+ * {@link org.apache.hc.client5.http.impl.DefaultHttpRequestRetryStrategy},
+ *
+ * <p>using the following list of non-retriable I/O exception classes:<br>
Review Comment:
Using `<br>` seems odd. And shouldn't these sentences be capitalized?
--
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
[GitHub] [iceberg] danielcweeks commented on a diff in pull request #8366: Core: Default to exponential retry strategy in REST client
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #8366:
URL: https://github.com/apache/iceberg/pull/8366#discussion_r1304513959
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -71,6 +74,12 @@ public class HTTPClient implements RESTClient {
@VisibleForTesting
static final String CLIENT_GIT_COMMIT_SHORT_HEADER = "X-Client-Git-Commit-Short";
+ private static final String RETRY_STRATEGY = "rest.client.retry-strategy";
+ private static final String REST_MAX_RETRIES = "rest.client.max-retries";
+ private static final String REST_RETRY_INTERVAL_MILLIS = "rest.client.retry-interval-millis";
Review Comment:
I feel like `rest.http.max-retries` is more clear. "Client" is ambiguous because this is a setting you configure on the RESTCatalog client (`http` makes it clear what the setting applies to).
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #8366: Core: Add exponential retry strategy to REST client
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8366:
URL: https://github.com/apache/iceberg/pull/8366#discussion_r1301882759
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -71,6 +74,12 @@ public class HTTPClient implements RESTClient {
@VisibleForTesting
static final String CLIENT_GIT_COMMIT_SHORT_HEADER = "X-Client-Git-Commit-Short";
+ private static final String RETRY_STRATEGY = "rest.client.retry-strategy";
+ private static final String REST_MAX_RETRIES = "rest.client.max-retries";
+ private static final String REST_RETRY_INTERVAL_MILLIS = "rest.client.retry-interval-millis";
Review Comment:
I'm ok either way, I just figured that having `client` in there makes it slightly more clear that we're targeting the REST HTTP client. Alternatively we could use `rest.http.max-retries`.
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #8366: Core: Default to exponential retry strategy in REST client
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #8366:
URL: https://github.com/apache/iceberg/pull/8366#discussion_r1324988933
##########
core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java:
##########
@@ -0,0 +1,145 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.ConnectException;
+import java.net.NoRouteToHostException;
+import java.net.UnknownHostException;
+import java.time.Instant;
+import java.util.Set;
+import java.util.concurrent.ThreadLocalRandom;
+import javax.net.ssl.SSLException;
+import org.apache.hc.client5.http.HttpRequestRetryStrategy;
+import org.apache.hc.client5.http.utils.DateUtils;
+import org.apache.hc.core5.concurrent.CancellableDependency;
+import org.apache.hc.core5.http.ConnectionClosedException;
+import org.apache.hc.core5.http.Header;
+import org.apache.hc.core5.http.HttpHeaders;
+import org.apache.hc.core5.http.HttpRequest;
+import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.Method;
+import org.apache.hc.core5.http.protocol.HttpContext;
+import org.apache.hc.core5.util.TimeValue;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+
+/**
+ * Defines an exponential HTTP request retry strategy and provides the same characteristics as the
+ * {@link org.apache.hc.client5.http.impl.DefaultHttpRequestRetryStrategy},
+ *
+ * <p>using the following list of non-retriable I/O exception classes:<br>
+ *
+ * <ul>
+ * <li>InterruptedIOException
+ * <li>UnknownHostException
+ * <li>ConnectException
+ * <li>ConnectionClosedException
+ * <li>NoRouteToHostException
+ * <li>SSLException
+ * </ul>
+ *
+ * and retriable HTTP status codes:<br>
+ *
+ * <ul>
+ * <li>SC_TOO_MANY_REQUESTS (429)
+ * <li>SC_SERVICE_UNAVAILABLE (503)
+ * </ul>
+ */
+class ExponentialHttpRequestRetryStrategy implements HttpRequestRetryStrategy {
Review Comment:
We need to update the LICENSE files to cover this addition.
--
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
[GitHub] [iceberg] rdblue merged pull request #8366: Core: Default to exponential retry strategy in REST client
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue merged PR #8366:
URL: https://github.com/apache/iceberg/pull/8366
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #8366: Core: Default to exponential retry strategy in REST client
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8366:
URL: https://github.com/apache/iceberg/pull/8366#discussion_r1325425077
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -96,6 +99,9 @@ private HTTPClient(
clientBuilder.addRequestInterceptorLast(requestInterceptor);
}
+ int maxRetries = PropertyUtil.propertyAsInt(properties, REST_MAX_RETRIES, 5);
Review Comment:
yes this was the intent to increase the retry
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #8366: Core: Add exponential retry strategy to REST client
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8366:
URL: https://github.com/apache/iceberg/pull/8366#discussion_r1301008271
##########
core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java:
##########
@@ -0,0 +1,145 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.ConnectException;
+import java.net.NoRouteToHostException;
+import java.net.UnknownHostException;
+import java.time.Instant;
+import java.util.Set;
+import java.util.concurrent.ThreadLocalRandom;
+import javax.net.ssl.SSLException;
+import org.apache.hc.client5.http.HttpRequestRetryStrategy;
+import org.apache.hc.client5.http.utils.DateUtils;
+import org.apache.hc.core5.concurrent.CancellableDependency;
+import org.apache.hc.core5.http.ConnectionClosedException;
+import org.apache.hc.core5.http.Header;
+import org.apache.hc.core5.http.HttpHeaders;
+import org.apache.hc.core5.http.HttpRequest;
+import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.Method;
+import org.apache.hc.core5.http.protocol.HttpContext;
+import org.apache.hc.core5.util.TimeValue;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+
+/**
+ * Defines an exponential HTTP request retry strategy and provides the same characteristics as the
+ * {@link org.apache.hc.client5.http.impl.DefaultHttpRequestRetryStrategy},
+ *
+ * <p>using the following list of non-retriable I/O exception classes:<br>
+ *
+ * <ul>
+ * <li>InterruptedIOException
+ * <li>UnknownHostException
+ * <li>ConnectException
+ * <li>ConnectionClosedException
+ * <li>NoRouteToHostException
+ * <li>SSLException
+ * </ul>
+ *
+ * and retriable HTTP status codes:<br>
+ *
+ * <ul>
+ * <li>SC_TOO_MANY_REQUESTS (429)
+ * <li>SC_SERVICE_UNAVAILABLE (503)
+ * </ul>
+ */
+class ExponentialHttpRequestRetryStrategy implements HttpRequestRetryStrategy {
Review Comment:
the entire logic and the handling is the same as in `DefaultHttpRequestRetryStrategy`. The only difference is the delay calculation in `getRetryInterval(..)`
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #8366: Core: Add exponential retry strategy to REST client
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8366:
URL: https://github.com/apache/iceberg/pull/8366#discussion_r1301879282
##########
core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java:
##########
@@ -0,0 +1,145 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.ConnectException;
+import java.net.NoRouteToHostException;
+import java.net.UnknownHostException;
+import java.time.Instant;
+import java.util.Set;
+import java.util.concurrent.ThreadLocalRandom;
+import javax.net.ssl.SSLException;
+import org.apache.hc.client5.http.HttpRequestRetryStrategy;
+import org.apache.hc.client5.http.utils.DateUtils;
+import org.apache.hc.core5.concurrent.CancellableDependency;
+import org.apache.hc.core5.http.ConnectionClosedException;
+import org.apache.hc.core5.http.Header;
+import org.apache.hc.core5.http.HttpHeaders;
+import org.apache.hc.core5.http.HttpRequest;
+import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.Method;
+import org.apache.hc.core5.http.protocol.HttpContext;
+import org.apache.hc.core5.util.TimeValue;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+
+/**
+ * Defines an exponential HTTP request retry strategy and provides the same characteristics as the
+ * {@link org.apache.hc.client5.http.impl.DefaultHttpRequestRetryStrategy},
+ *
+ * <p>using the following list of non-retriable I/O exception classes:<br>
+ *
+ * <ul>
+ * <li>InterruptedIOException
+ * <li>UnknownHostException
+ * <li>ConnectException
+ * <li>ConnectionClosedException
+ * <li>NoRouteToHostException
+ * <li>SSLException
+ * </ul>
+ *
+ * and retriable HTTP status codes:<br>
+ *
+ * <ul>
+ * <li>SC_TOO_MANY_REQUESTS (429)
+ * <li>SC_SERVICE_UNAVAILABLE (503)
+ * </ul>
+ */
+class ExponentialHttpRequestRetryStrategy implements HttpRequestRetryStrategy {
Review Comment:
I added a comment that mentions where most of the code is coming from
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #8366: Core: Add exponential retry strategy to REST client
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #8366:
URL: https://github.com/apache/iceberg/pull/8366#discussion_r1301829580
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -71,6 +74,12 @@ public class HTTPClient implements RESTClient {
@VisibleForTesting
static final String CLIENT_GIT_COMMIT_SHORT_HEADER = "X-Client-Git-Commit-Short";
+ private static final String RETRY_STRATEGY = "rest.client.retry-strategy";
+ private static final String REST_MAX_RETRIES = "rest.client.max-retries";
+ private static final String REST_RETRY_INTERVAL_MILLIS = "rest.client.retry-interval-millis";
Review Comment:
Do we need `client` in all these properties? Is it clear what `client` refers to and that it is distinct from `rest`?
--
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
[GitHub] [iceberg] bryanck commented on a diff in pull request #8366: Core: Default to exponential retry strategy in REST client
Posted by "bryanck (via GitHub)" <gi...@apache.org>.
bryanck commented on code in PR #8366:
URL: https://github.com/apache/iceberg/pull/8366#discussion_r1324913214
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -96,6 +99,9 @@ private HTTPClient(
clientBuilder.addRequestInterceptorLast(requestInterceptor);
}
+ int maxRetries = PropertyUtil.propertyAsInt(properties, REST_MAX_RETRIES, 5);
Review Comment:
This is a higher retry count than before with the default (which is 1 retry I believe), just wanted to double check that was the intent.
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #8366: Core: Default to exponential retry strategy in REST client
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8366:
URL: https://github.com/apache/iceberg/pull/8366#discussion_r1325424784
##########
core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java:
##########
@@ -0,0 +1,145 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.ConnectException;
+import java.net.NoRouteToHostException;
+import java.net.UnknownHostException;
+import java.time.Instant;
+import java.util.Set;
+import java.util.concurrent.ThreadLocalRandom;
+import javax.net.ssl.SSLException;
+import org.apache.hc.client5.http.HttpRequestRetryStrategy;
+import org.apache.hc.client5.http.utils.DateUtils;
+import org.apache.hc.core5.concurrent.CancellableDependency;
+import org.apache.hc.core5.http.ConnectionClosedException;
+import org.apache.hc.core5.http.Header;
+import org.apache.hc.core5.http.HttpHeaders;
+import org.apache.hc.core5.http.HttpRequest;
+import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.Method;
+import org.apache.hc.core5.http.protocol.HttpContext;
+import org.apache.hc.core5.util.TimeValue;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+
+/**
+ * Defines an exponential HTTP request retry strategy and provides the same characteristics as the
+ * {@link org.apache.hc.client5.http.impl.DefaultHttpRequestRetryStrategy},
+ *
+ * <p>using the following list of non-retriable I/O exception classes:<br>
+ *
+ * <ul>
+ * <li>InterruptedIOException
+ * <li>UnknownHostException
+ * <li>ConnectException
+ * <li>ConnectionClosedException
+ * <li>NoRouteToHostException
+ * <li>SSLException
+ * </ul>
+ *
+ * and retriable HTTP status codes:<br>
+ *
+ * <ul>
+ * <li>SC_TOO_MANY_REQUESTS (429)
+ * <li>SC_SERVICE_UNAVAILABLE (503)
+ * </ul>
+ */
+class ExponentialHttpRequestRetryStrategy implements HttpRequestRetryStrategy {
Review Comment:
I added a note to the LICENSE file
--
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
[GitHub] [iceberg] rdblue commented on a diff in pull request #8366: Core: Add exponential retry strategy to REST client
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #8366:
URL: https://github.com/apache/iceberg/pull/8366#discussion_r1301819243
##########
core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java:
##########
@@ -0,0 +1,145 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.ConnectException;
+import java.net.NoRouteToHostException;
+import java.net.UnknownHostException;
+import java.time.Instant;
+import java.util.Set;
+import java.util.concurrent.ThreadLocalRandom;
+import javax.net.ssl.SSLException;
+import org.apache.hc.client5.http.HttpRequestRetryStrategy;
+import org.apache.hc.client5.http.utils.DateUtils;
+import org.apache.hc.core5.concurrent.CancellableDependency;
+import org.apache.hc.core5.http.ConnectionClosedException;
+import org.apache.hc.core5.http.Header;
+import org.apache.hc.core5.http.HttpHeaders;
+import org.apache.hc.core5.http.HttpRequest;
+import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.Method;
+import org.apache.hc.core5.http.protocol.HttpContext;
+import org.apache.hc.core5.util.TimeValue;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+
+/**
+ * Defines an exponential HTTP request retry strategy and provides the same characteristics as the
+ * {@link org.apache.hc.client5.http.impl.DefaultHttpRequestRetryStrategy},
+ *
+ * <p>using the following list of non-retriable I/O exception classes:<br>
+ *
+ * <ul>
+ * <li>InterruptedIOException
+ * <li>UnknownHostException
+ * <li>ConnectException
+ * <li>ConnectionClosedException
+ * <li>NoRouteToHostException
+ * <li>SSLException
+ * </ul>
+ *
+ * and retriable HTTP status codes:<br>
+ *
+ * <ul>
+ * <li>SC_TOO_MANY_REQUESTS (429)
+ * <li>SC_SERVICE_UNAVAILABLE (503)
+ * </ul>
+ */
+class ExponentialHttpRequestRetryStrategy implements HttpRequestRetryStrategy {
Review Comment:
Where did the code come from? We need to document that, even if it is from another ASF project.
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #8366: Core: Add exponential retry strategy to REST client
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8366:
URL: https://github.com/apache/iceberg/pull/8366#discussion_r1301879478
##########
core/src/main/java/org/apache/iceberg/rest/ExponentialHttpRequestRetryStrategy.java:
##########
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.rest;
+
+import java.io.IOException;
+import java.io.InterruptedIOException;
+import java.net.ConnectException;
+import java.net.NoRouteToHostException;
+import java.net.UnknownHostException;
+import java.time.Instant;
+import java.util.Set;
+import java.util.concurrent.ThreadLocalRandom;
+import javax.net.ssl.SSLException;
+import org.apache.hc.client5.http.HttpRequestRetryStrategy;
+import org.apache.hc.client5.http.utils.DateUtils;
+import org.apache.hc.core5.concurrent.CancellableDependency;
+import org.apache.hc.core5.http.ConnectionClosedException;
+import org.apache.hc.core5.http.Header;
+import org.apache.hc.core5.http.HttpHeaders;
+import org.apache.hc.core5.http.HttpRequest;
+import org.apache.hc.core5.http.HttpResponse;
+import org.apache.hc.core5.http.HttpStatus;
+import org.apache.hc.core5.http.Method;
+import org.apache.hc.core5.http.protocol.HttpContext;
+import org.apache.hc.core5.util.TimeValue;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
+
+/**
+ * Defines an exponential HTTP request retry strategy and provides the same characteristics as the
+ * {@link org.apache.hc.client5.http.impl.DefaultHttpRequestRetryStrategy},
+ *
+ * <p>using the following list of non-retriable I/O exception classes:<br>
Review Comment:
fixed
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #8366: Core: Default to exponential retry strategy in REST client
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #8366:
URL: https://github.com/apache/iceberg/pull/8366#discussion_r1304567154
##########
core/src/main/java/org/apache/iceberg/rest/HTTPClient.java:
##########
@@ -71,6 +74,12 @@ public class HTTPClient implements RESTClient {
@VisibleForTesting
static final String CLIENT_GIT_COMMIT_SHORT_HEADER = "X-Client-Git-Commit-Short";
+ private static final String RETRY_STRATEGY = "rest.client.retry-strategy";
+ private static final String REST_MAX_RETRIES = "rest.client.max-retries";
+ private static final String REST_RETRY_INTERVAL_MILLIS = "rest.client.retry-interval-millis";
Review Comment:
I've updated this to `rest.http.max-retries`
--
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
[GitHub] [iceberg] rdblue commented on pull request #8366: Core: Default to exponential retry strategy in REST client
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on PR #8366:
URL: https://github.com/apache/iceberg/pull/8366#issuecomment-1720239972
Thanks, @nastra!
--
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