[WAGON-526] making the retry handle of http client configurable #37

rmannibucau · 2017-12-05T11:10:56Z

No description provided.

jkff · 2017-12-05T17:18:10Z

Very nice! Could you comment more on how to use this in practice to improve reliability w.r.t. common network issues? Is the default behavior changed?

rmannibucau · 2017-12-05T17:37:33Z

@jkff default didnt change (backwd compat but open to suggestions)

idea is to add system properties to maven (MAVEN_OPTS). Here are some samples:

-Dmaven.wagon.http.retryhandler.class=default
-Dmaven.wagon.http.retryhandler.count=5
-Dmaven.wagon.http.retryhandler.requestSentEnabled=false #default
-Dmaven.wagon.http.retryhandler.nonRetryableClasses=javax.net.ssl.SSLException,java.net.UnknownHostException

Note that -Dmaven.wagon.http.retryhandler.class=default can be switched to -Dmaven.wagon.http.retryhandler.class=standard to use the standard retry handler impl. using a fqn class name of a http retry handler it will replace the default impl with your custom class (you must add the jar to maven libs).

jkff · 2017-12-06T03:38:58Z

I see. So, for example, what's the shortest way to configure this to simply retry everything, including socket exceptions, host resolution exceptions etc? (what we want in Beam builds when downloading artifacts) Would it be -Dmaven.wagon.http.retryhandler.class=standard ?

rmannibucau · 2017-12-06T06:07:22Z

I guess

-Dmaven.wagon.http.retryhandler.class=default
-Dmaven.wagon.http.retryhandler.nonRetryableClasses=java.net.UnknownHostException

can be a good start

jkff · 2017-12-06T17:27:53Z

Wait: the default one would give current behavior, which doesn't retry on socket errors. Did you mean -Dmaven.wagon.http.retryhandler.class=standard perhaps, and without the nonRetryableClasses argument (cause I'd prefer UnknownHost to also be retried, it can happen due to transient network issues)?
It's a bit confusing that default and standard have such different meanings...

Anyway: hope that some of the maintainers can chime in perhaps?

rmannibucau · 2017-12-06T17:34:25Z

Small misunderstanding: default behavior is the current one but default (same for standard) clasq value actually references the name of the impl -
with a shortname to be human feiendly - and default impl supports retryable exception classes configuration.

olamy · 2017-12-07T04:33:04Z

interesting feature. I wonder you could add some documentation maybe here: http://maven.apache.org/wagon/wagon-providers/wagon-http/

rmannibucau · 2017-12-07T08:40:57Z

@olamy updated site apt

michael-o · 2017-12-07T10:25:29Z

There is bit of a chaos now. I know that the Deploy Plugin supports something similar and people are requesting the same feature for this problem area. I'll collect and have a look at it.

jkff · 2018-06-28T22:21:48Z

Is there anything going on with this PR? If it's stale, it might be reasonable to close it and reopen later. We're no longer even using Maven in our project so this PR is not a priority for us.

rmannibucau · 2018-06-29T04:20:57Z

@jkff sure, this was done here to not be beam specific since it happens for multiple projects

olamy · 2018-06-29T04:26:38Z

@rmannibucau maybe you can rebase as I can see some conflicts

michael-o · 2018-06-29T06:38:21Z

My bad, I lost completely track of this.

michael-o

Looks quite good. Some oddities remain.

michael-o · 2018-08-19T19:25:44Z

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

+            System.getProperty( "maven.wagon.http.retryhandler.class", "auto" );
+
+    /**
+     * true if it's OK to retry non-idempotent requests that have been sent.


What makes you think that this applies to non-idemponent methods? The HttpRequestRetryHandler doesn't even mention them.

michael-o · 2018-08-19T19:26:18Z

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

+     * The type of the retry handler, default to DefaultHttpRequestRetryHandler.
+     * Values can be default, standard (StandardHttpRequestRetryHandler), or a fully qualified name class.
+     *
+     * @since 3.0.1


Please update this throughout to 3.2 if this one will be merged.

michael-o · 2018-08-19T19:30:40Z

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

+     * @since 3.0.1
+     */
+    private static final String RETRY_HANDLER_CLASS =
+            System.getProperty( "maven.wagon.http.retryhandler.class", "auto" );


This looks wrong to me. Having non-recoverable methods to be retriable by default. We have to be idiotproof and support here standard with StandardHttpRequestRetryHandler.

michael-o · 2018-08-19T19:30:55Z

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

+                };
+            case "standard":
+                return new StandardHttpRequestRetryHandler( RETRY_HANDLER_COUNT, RETRY_HANDLER_REQUEST_SENT_ENABLED );
+            case "auto":


I don't accept this for the reasons above.

changing to standard

This needs to be dropped completely.

michael-o · 2018-08-19T19:35:45Z

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

+        switch ( RETRY_HANDLER_CLASS )
+        {
+            case "default":
+                if ( DEFAULT_NON_RETRYABLE_CLASSES.equals( RETRY_HANDLER_EXCEPTIONS ) )


This is deemed to fail if the comparison does not happen in order.

michael-o · 2018-08-19T19:38:41Z

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

+                    return new DefaultHttpRequestRetryHandler(
+                            RETRY_HANDLER_COUNT, RETRY_HANDLER_REQUEST_SENT_ENABLED );
+                }
+                return new DefaultHttpRequestRetryHandler(


Is is really smart to provide something like this for unsafe methods by default?

changed the default so guess it is ok now

michael-o · 2018-08-19T19:41:13Z

wagon-providers/wagon-http/src/site/apt/index.apt

+
+    * which implementation to use. `auto` will use `DefaultHttpRequestRetryHandler.INSTANCE`.
+    * `default` will use a new instance of `DefaultHttpRequestRetryHandler` respecting `requestSentEnabled`, `count` and `nonRetryableClasses`.
+    * `standard` will use an instance of `StandardHttpRequestRetryHandler` which assumes that all requested HTTP methods which should be idempotent according to RFC-2616 are in fact idempotent and can be retried. According to RFC-2616 section 9.1.2 the idempotent HTTP methods are: GET, HEAD, PUT, DELETE, OPTIONS, and TRACE. This implementation respects `requestSentEnabled` and `count`. 


It's "RFC xyz". No hyphen involved.

michael-o · 2018-08-19T19:41:46Z

wagon-providers/wagon-http/src/site/apt/index.apt

+
+    * which implementation to use. `auto` will use `DefaultHttpRequestRetryHandler.INSTANCE`.
+    * `default` will use a new instance of `DefaultHttpRequestRetryHandler` respecting `requestSentEnabled`, `count` and `nonRetryableClasses`.
+    * `standard` will use an instance of `StandardHttpRequestRetryHandler` which assumes that all requested HTTP methods which should be idempotent according to RFC-2616 are in fact idempotent and can be retried. According to RFC-2616 section 9.1.2 the idempotent HTTP methods are: GET, HEAD, PUT, DELETE, OPTIONS, and TRACE. This implementation respects `requestSentEnabled` and `count`. 


Don't repeat the same as the Javadoc of that class. Simply link to it.

michael-o · 2018-08-19T19:42:51Z

...on-http/src/test/java/org/apache/maven/wagon/providers/http/AbstractHttpClientWagonTest.java

+            @Override
+            public void run()
+            {
+                final HttpRequestRetryHandler handler = getCurrentHttpClient();


It is weird that getCurrentHttpClient() does not return a client actually.

michael-o · 2018-08-19T19:43:29Z

...on-http/src/test/java/org/apache/maven/wagon/providers/http/AbstractHttpClientWagonTest.java

+                {
+                    final Field nonRetriableClasses = handler.getClass().getSuperclass()
+                            .getDeclaredField( "nonRetriableClasses" );
+                    if (!nonRetriableClasses.isAccessible())


Will this work with Java 10/11?

If modules are opened I think so

rmannibucau · 2018-08-20T09:04:10Z

Updated the PR accordingly the comments, let me know if it is better this way for you

michael-o

I am still not convinced by stanard/default. From user perspective we have default and standard, what? Isn't it the same? What we have to provide has to be idiotsafe, idempotent. I currently trying to figure out what the benefit of the default is and if whether standard will cover 95% of all cases. The user still can provide a custom implementation.

michael-o · 2018-08-20T09:28:22Z

wagon-providers/wagon-http/src/site/apt/index.apt

+
+ []
+
+ Since version 3.0.1, the retry handler can be configured as well with system properties:


Version seems wrong

michael-o · 2018-08-20T09:28:34Z

wagon-providers/wagon-http/src/site/apt/index.apt

+
+ * <<<maven.wagon.http.retryhandler.class>>> supports this set of values:
+
+    * which implementation to use. `auto` will use `DefaultHttpRequestRetryHandler.INSTANCE`.


Isn't auto gone?

michael-o · 2018-08-20T09:29:27Z

wagon-providers/wagon-http/src/site/apt/index.apt

+ * <<<maven.wagon.http.retryhandler.count>>> = number of retries for `default` or `standard` implementations.
+
+ * <<<maven.wagon.http.retryhandler.nonRetryableClasses>>> = the comma separated list of classes bypassing the retries (only the `default` implementation).
+ Default being `java.io.InterruptedIOException,java.io.UnknownHostException,java.io.ConnectException,java.io.SSLException`


There is no default anymore provided by us.

… the doc

rmannibucau

Updated the doc

michael-o · 2018-08-20T12:22:54Z

Waiting for all spots to be resolved and a JIRA issue. Will then review, test and merge.

rmannibucau · 2018-08-20T12:49:16Z

@michael-o added a jira, think I adressed the issues, please let me know if I missed some

michael-o · 2018-08-20T13:07:19Z

You completely missed my review request:

I am still not convinced by stanard/default. From user perspective we have default and standard, what? Isn't it the same? What we have to provide has to be idiotsafe, idempotent. I currently trying to figure out what the benefit of the default is and if whether standard will cover 95% of all cases. The user still can provide a custom implementation.

rmannibucau · 2018-08-20T14:29:41Z

@michael-o oh oki. Yes it makes sense, it basically exposes the known impl of httpclient in maven and match the same use cases. Rational was to never require a custom impl in 99% of the cases cause it is quite impacting to do it compared to configuring the standard or default impls.

michael-o · 2018-08-20T14:45:09Z

Alright, I will have at this later this day. I think there is still room for some improvement, but I like the big picture so far.

michael-o · 2018-08-20T19:30:23Z

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

+     * Comma separated list of non retryable classes.
+     * Note: only used for default retry handler.
+     *
+     * @since 3.0.1


Still wrong.

michael-o · 2018-08-20T19:30:58Z

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

+                };
+            case "standard":
+                return new StandardHttpRequestRetryHandler( RETRY_HANDLER_COUNT, RETRY_HANDLER_REQUEST_SENT_ENABLED );
+            case "auto":


This needs to be dropped completely.

michael-o · 2018-08-20T19:39:20Z

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

@@ -354,6 +360,92 @@ private static PoolingHttpClientConnectionManager createConnManager()
        return connManager;
    }

+    /**
+     * The type of the retry handler, default to DefaultHttpRequestRetryHandler.
+     * Values can be default, standard (StandardHttpRequestRetryHandler), or a fully qualified name class.


This is inconsistent in style. Please use: label (class name), or a fully qualified class name.

michael-o · 2018-08-20T19:41:00Z

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

+            System.getProperty( "maven.wagon.http.retryhandler.class", "standard" );
+
+    /**
+     * true if it's OK to retry non-idempotent requests that have been sent.


This statement is not true. The original documentation does not say this. Even the default implementation will stop before this value is read if the request ist not idempotent.

michael-o · 2018-08-20T19:42:07Z

...on-http-shared/src/main/java/org/apache/maven/wagon/shared/http/AbstractHttpClientWagon.java

+        switch ( RETRY_HANDLER_CLASS )
+        {
+            case "default":
+                if ( RETRY_HANDLER_EXCEPTIONS == null )


I'd rather rely on StringUtils#isEmpty().

michael-o · 2018-08-20T19:49:23Z

...on-http/src/test/java/org/apache/maven/wagon/providers/http/AbstractHttpClientWagonTest.java

+        final URLClassLoader loader = new URLClassLoader( urls.toArray( new URL[paths.length ] ) , new ClassLoader()
+        {
+            @Override
+            protected Class<?> loadClass(final String name, final boolean resolve) throws ClassNotFoundException


spaces missing

michael-o · 2018-08-20T19:49:37Z

...on-http/src/test/java/org/apache/maven/wagon/providers/http/AbstractHttpClientWagonTest.java

+            {
+                if ( name.startsWith( "org.apache.maven.wagon.shared.http" ) )
+                {
+                    throw new ClassNotFoundException(name);


space missing

michael-o · 2018-08-20T19:49:41Z

...on-http/src/test/java/org/apache/maven/wagon/providers/http/AbstractHttpClientWagonTest.java

+                {
+                    throw new ClassNotFoundException(name);
+                }
+                return super.loadClass(name, resolve);


space missing

michael-o · 2018-08-20T19:49:45Z

...on-http/src/test/java/org/apache/maven/wagon/providers/http/AbstractHttpClientWagonTest.java

+        });
+        final Thread thread = Thread.currentThread();
+        final ClassLoader contextClassLoader = thread.getContextClassLoader();
+        thread.setContextClassLoader(loader);


space missing

michael-o · 2018-08-20T19:49:58Z

...on-http/src/test/java/org/apache/maven/wagon/providers/http/AbstractHttpClientWagonTest.java

+        finally
+        {
+            loader.close();
+            thread.setContextClassLoader(contextClassLoader);


space missing

michael-o · 2018-08-21T07:40:59Z

Looks very good now. I run push a test branch later this day.

michael-o · 2018-08-24T17:48:54Z

I have pushed a cleaned up branch. Please have a look. If all is fine, I will merge.

rmannibucau · 2018-08-26T14:31:38Z

@michael-o it is perfect, thanks a lot!

michael-o requested changes Aug 19, 2018

View reviewed changes

adding doc for retryhandler configuration

ce7802e

rmannibucau force-pushed the fb/configurable-http-retry-policy branch from 6f68473 to ce7802e Compare August 20, 2018 08:59

avoid to duplicate part of DefaultHttpRequestRetryHandler

039e8e1

michael-o requested changes Aug 20, 2018

View reviewed changes

rmannibucau added 2 commits August 20, 2018 14:15

fixing doc version

268e0b3

dropping hardcoded default list of exception foe the retry handler in…

12c36ff

… the doc

rmannibucau commented Aug 20, 2018

View reviewed changes

rmannibucau changed the title ~~making the retry handle of http client configurable~~ [WAGON-526] making the retry handle of http client configurable Aug 20, 2018

michael-o requested changes Aug 20, 2018

View reviewed changes

dropping 'auto' + style

b97ad5f

asfgit closed this in f920bc1 Aug 26, 2018


		[]

		Since version 3.0.1, the retry handler can be configured as well with system properties:


		* <<<maven.wagon.http.retryhandler.class>>> supports this set of values:

		* which implementation to use. `auto` will use `DefaultHttpRequestRetryHandler.INSTANCE`.

[WAGON-526] making the retry handle of http client configurable #37

[WAGON-526] making the retry handle of http client configurable #37

Conversation

rmannibucau commented Dec 5, 2017

jkff commented Dec 5, 2017

rmannibucau commented Dec 5, 2017

jkff commented Dec 6, 2017

rmannibucau commented Dec 6, 2017

jkff commented Dec 6, 2017

rmannibucau commented Dec 6, 2017

olamy commented Dec 7, 2017

rmannibucau commented Dec 7, 2017

michael-o commented Dec 7, 2017

jkff commented Jun 28, 2018

rmannibucau commented Jun 29, 2018

olamy commented Jun 29, 2018

michael-o commented Jun 29, 2018

michael-o left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmannibucau commented Aug 20, 2018

michael-o left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmannibucau left a comment

Choose a reason for hiding this comment

michael-o commented Aug 20, 2018

rmannibucau commented Aug 20, 2018

michael-o commented Aug 20, 2018

rmannibucau commented Aug 20, 2018

michael-o commented Aug 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-o commented Aug 21, 2018

michael-o commented Aug 24, 2018

rmannibucau commented Aug 26, 2018

michael-o commented Aug 20, 2018 •

edited