Skip to content

Commit 8010921

Browse files
dwalluckdwalluck
committed
NCL-6509: Improve XML-RPC retry handling
Extend the XML-RPC retry logic to retry on `NoHttpResponseException` in addition to `ConnectException`. Since we can't know in the case of a `NoHttpResponseException` whether the server processed the request, we check the method name to determine if it is safe to retry. We assume that API calls starting with the words get, list, query, or has are read-only and safe to retry and all others are not. Change from linear backoff to exponential backoff to give the server more time.
1 parent 938eab6 commit 8010921

3 files changed

Lines changed: 130 additions & 9 deletions

File tree

‎src/main/java/com/redhat/red/build/koji/http/httpclient4/HC4SyncObjectClient.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ public <T> T call( final Object request, final Class<T> responseType, final UrlB
6666
{
6767
if ( metricRegistry == null )
6868
{
69-
return RetryUtils.withRetry( () -> doCall( request, responseType, urlBuilder, requestModifier ) );
69+
return RetryUtils.withRetry( () -> doCall( request, responseType, urlBuilder, requestModifier ), request );
7070
}
7171

7272
// Apply global and per request metric
@@ -76,7 +76,7 @@ public <T> T call( final Object request, final Class<T> responseType, final UrlB
7676
{
7777
try
7878
{
79-
return RetryUtils.withRetry( () -> doCall( request, responseType, urlBuilder, requestModifier ) );
79+
return RetryUtils.withRetry( () -> doCall( request, responseType, urlBuilder, requestModifier ), request );
8080
}
8181
finally
8282
{

‎src/main/java/com/redhat/red/build/koji/http/httpclient4/RetryUtils.java‎

Lines changed: 54 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,31 +15,40 @@
1515
*/
1616
package com.redhat.red.build.koji.http.httpclient4;
1717

18+
import com.redhat.red.build.koji.model.xmlrpc.KojiMultiCallObj;
19+
import com.redhat.red.build.koji.model.xmlrpc.messages.MultiCallRequest;
20+
import org.apache.http.NoHttpResponseException;
21+
import org.commonjava.rwx.anno.Request;
1822
import org.commonjava.rwx.error.XmlRpcException;
1923
import org.slf4j.Logger;
2024
import org.slf4j.LoggerFactory;
2125

2226
import java.net.ConnectException;
27+
import java.util.List;
28+
29+
import static com.redhat.red.build.koji.model.xmlrpc.messages.Constants.MULTI_CALL;
2330

2431
public class RetryUtils
2532
{
2633
private static final Logger logger = LoggerFactory.getLogger( RetryUtils.class );
2734

2835
public static final int DEFAULT_RETRY_COUNT = 3;
2936

30-
public static final long DEFAULT_WAIT_SECONDS = 10;
37+
public static final long DEFAULT_WAIT_SECONDS = 10L;
38+
39+
public static final long MAX_WAIT_SECONDS = 60L;
3140

3241
public interface CallToRetry<R>
3342
{
3443
R process() throws XmlRpcException;
3544
}
3645

37-
public static <T> T withRetry( CallToRetry call ) throws XmlRpcException
46+
public static <T> T withRetry( CallToRetry call, Object request ) throws XmlRpcException
3847
{
39-
return withRetry( DEFAULT_RETRY_COUNT, DEFAULT_WAIT_SECONDS, call );
48+
return withRetry( DEFAULT_RETRY_COUNT, DEFAULT_WAIT_SECONDS, call, request );
4049
}
4150

42-
public static <T> T withRetry( int retryCount, long interval, CallToRetry call ) throws XmlRpcException
51+
public static <T> T withRetry( int retryCount, long interval, CallToRetry call, Object request ) throws XmlRpcException
4352
{
4453
int count = 0;
4554
while ( true )
@@ -55,12 +64,16 @@ public static <T> T withRetry( int retryCount, long interval, CallToRetry call )
5564
throw e;
5665
}
5766

58-
if ( e.getCause() instanceof ConnectException )
67+
Throwable cause = e.getCause();
68+
69+
if ( cause instanceof ConnectException || ( cause instanceof NoHttpResponseException && isSafeToRetry( request ) ) )
5970
{
60-
logger.info( "ConnectException {}/{}, Waiting for {} second(s) before next retry ...", e.getCause().getClass(), e.getCause().getMessage(), interval );
71+
long seconds = Math.min( interval << ( count - 1 ), MAX_WAIT_SECONDS );
72+
logger.info( "ConnectException {}/{}, Waiting for {} second(s) before next retry ...", cause.getClass(), cause.getMessage(), seconds );
73+
6174
try
6275
{
63-
Thread.sleep( interval * 1000l );
76+
Thread.sleep( seconds * 1000L );
6477
}
6578
catch ( InterruptedException ex )
6679
{
@@ -75,4 +88,38 @@ public static <T> T withRetry( int retryCount, long interval, CallToRetry call )
7588
}
7689
}
7790

91+
static boolean isSafeToRetry( Object obj )
92+
{
93+
Request request = obj.getClass().getAnnotation( Request.class );
94+
95+
if ( request == null )
96+
{
97+
return false;
98+
}
99+
100+
String method = request.method();
101+
102+
if ( MULTI_CALL.equals( method ) )
103+
{
104+
MultiCallRequest multiCallRequest = (MultiCallRequest) obj;
105+
List<KojiMultiCallObj> multiCallObjs = multiCallRequest.getMultiCallObjs();
106+
107+
for ( KojiMultiCallObj multiCallObj : multiCallObjs )
108+
{
109+
if ( !isSafeCall( multiCallObj.getMethodName() ) )
110+
{
111+
return false;
112+
}
113+
}
114+
115+
return true;
116+
}
117+
118+
return isSafeCall( method );
119+
}
120+
121+
static boolean isSafeCall( String methodName )
122+
{
123+
return methodName.startsWith( "get" ) || methodName.startsWith( "list" ) || methodName.startsWith( "query" ) || methodName.startsWith( "has" );
124+
}
78125
}

‎src/test/java/com/redhat/red/build/koji/http/httpclient4/RetryUtilsTest.java‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
/*
2+
* Copyright (C) 2015 Red Hat, Inc.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package com.redhat.red.build.koji.http.httpclient4;
17+
18+
import com.redhat.red.build.koji.KojiClientUtils;
19+
import com.redhat.red.build.koji.model.xmlrpc.messages.AllPermissionsRequest;
20+
import com.redhat.red.build.koji.model.xmlrpc.messages.ApiVersionRequest;
21+
import com.redhat.red.build.koji.model.xmlrpc.messages.CGInlinedImportRequest;
22+
import com.redhat.red.build.koji.model.xmlrpc.messages.CGUploadedImportRequest;
23+
import com.redhat.red.build.koji.model.xmlrpc.messages.CheckPermissionRequest;
24+
import com.redhat.red.build.koji.model.xmlrpc.messages.Constants;
25+
import com.redhat.red.build.koji.model.xmlrpc.messages.GetBuildTypeRequest;
26+
import com.redhat.red.build.koji.model.xmlrpc.messages.ListBuildTypesRequest;
27+
import com.redhat.red.build.koji.model.xmlrpc.messages.LoginRequest;
28+
import com.redhat.red.build.koji.model.xmlrpc.messages.LoginResponse;
29+
import com.redhat.red.build.koji.model.xmlrpc.messages.LogoutRequest;
30+
import com.redhat.red.build.koji.model.xmlrpc.messages.MultiCallRequest;
31+
import com.redhat.red.build.koji.model.xmlrpc.messages.QueryRpmSigsRequest;
32+
import org.junit.Test;
33+
34+
import java.util.Collections;
35+
import java.util.List;
36+
37+
import static org.commonjava.rwx.vocab.Nil.NIL_VALUE;
38+
import static org.hamcrest.CoreMatchers.is;
39+
import static org.hamcrest.MatcherAssert.assertThat;
40+
41+
public class RetryUtilsTest
42+
{
43+
@Test
44+
public void testIsSafeToRetry()
45+
{
46+
assertThat( RetryUtils.isSafeToRetry( new AllPermissionsRequest() ), is( true ) );
47+
assertThat( RetryUtils.isSafeToRetry( new ApiVersionRequest() ), is( true ) );
48+
assertThat( RetryUtils.isSafeToRetry( new CheckPermissionRequest() ), is( true ) );
49+
assertThat( RetryUtils.isSafeToRetry( new GetBuildTypeRequest() ), is( true ) );
50+
assertThat( RetryUtils.isSafeToRetry( new ListBuildTypesRequest() ), is( true ) );
51+
assertThat( RetryUtils.isSafeToRetry( new QueryRpmSigsRequest() ), is( true ) );
52+
}
53+
54+
@Test
55+
public void testIsNotSafeToRetry()
56+
{
57+
assertThat( RetryUtils.isSafeToRetry( new CGInlinedImportRequest() ), is( false ) );
58+
assertThat( RetryUtils.isSafeToRetry( new CGUploadedImportRequest() ), is( false ) );
59+
assertThat( RetryUtils.isSafeToRetry( new LoginRequest() ), is( false ) );
60+
assertThat( RetryUtils.isSafeToRetry( new LogoutRequest() ), is( false ) );
61+
assertThat( RetryUtils.isSafeToRetry( new LoginResponse() ), is( false ) );
62+
}
63+
64+
@Test
65+
public void testRetryWithMultiCall()
66+
{
67+
List<Object> args = Collections.singletonList( NIL_VALUE );
68+
MultiCallRequest req1 = KojiClientUtils.buildMultiCallRequest( Constants.GET_API_VERSION, args );
69+
assertThat( RetryUtils.isSafeToRetry( req1 ), is( true ) );
70+
MultiCallRequest req2 = KojiClientUtils.buildMultiCallRequest( Constants.LOGOUT, args );
71+
assertThat( RetryUtils.isSafeToRetry( req2 ), is( false ) );
72+
73+
}
74+
}

0 commit comments

Comments
 (0)