You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@servicecomb.apache.org by li...@apache.org on 2022/01/21 03:36:33 UTC
[servicecomb-java-chassis] 02/05: [SCB-2373]fix problems for retry in exceptions, loadbalance and improve logs
This is an automated email from the ASF dual-hosted git repository.
liubao pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/servicecomb-java-chassis.git
commit 6e39d6b8d01c117c966b9a941e6de0b743970a37
Author: liubao68 <bi...@qq.com>
AuthorDate: Wed Jan 19 17:38:36 2022 +0800
[SCB-2373]fix problems for retry in exceptions, loadbalance and improve logs
---
.../servicecomb/core/governance/RetryContext.java | 4 +-
.../core/governance/ServiceCombRetryExtension.java | 35 ++--
.../core/provider/consumer/InvokerUtils.java | 192 +++++++++----------
.../core/provider/consumer/TestInvokerUtils.java | 203 ++-------------------
.../src/main/resources/microservice.yaml | 9 +-
.../src/main/resources/microservice.yaml | 10 +-
.../governance/handler/RetryHandler.java | 13 +-
.../governance/handler/ext/RetryExtension.java | 52 +++++-
.../servicecomb/governance/MockRetryExtension.java | 7 +-
.../governance/handler/ext/RetryExtensionTest.java | 87 ++++++++-
.../servicecomb/bizkeeper/BizkeeperCommand.java | 9 +-
.../loadbalance/LoadbalanceHandler.java | 12 +-
.../servicecomb/loadbalance/ServiceCombServer.java | 7 +-
.../servicecomb/swagger/invocation/Response.java | 5 +
14 files changed, 293 insertions(+), 352 deletions(-)
diff --git a/core/src/main/java/org/apache/servicecomb/core/governance/RetryContext.java b/core/src/main/java/org/apache/servicecomb/core/governance/RetryContext.java
index 6fa4d7d..bdb4d04 100644
--- a/core/src/main/java/org/apache/servicecomb/core/governance/RetryContext.java
+++ b/core/src/main/java/org/apache/servicecomb/core/governance/RetryContext.java
@@ -17,7 +17,9 @@
package org.apache.servicecomb.core.governance;
public class RetryContext {
- public static final String RETRY_CONTEXT = "x-servicecomb-retry";
+ public static final String RETRY_CONTEXT = "x-context-retry";
+
+ public static final String RETRY_LOAD_BALANCE = "x-context-retry-loadbalance";
private boolean retry;
diff --git a/core/src/main/java/org/apache/servicecomb/core/governance/ServiceCombRetryExtension.java b/core/src/main/java/org/apache/servicecomb/core/governance/ServiceCombRetryExtension.java
index cb90602..ecc5ed9 100644
--- a/core/src/main/java/org/apache/servicecomb/core/governance/ServiceCombRetryExtension.java
+++ b/core/src/main/java/org/apache/servicecomb/core/governance/ServiceCombRetryExtension.java
@@ -17,18 +17,13 @@
package org.apache.servicecomb.core.governance;
-import io.vertx.core.VertxException;
+import javax.ws.rs.core.Response.Status;
import org.apache.servicecomb.governance.handler.ext.AbstractRetryExtension;
import org.apache.servicecomb.swagger.invocation.Response;
import org.apache.servicecomb.swagger.invocation.exception.InvocationException;
import org.springframework.stereotype.Component;
-import java.io.IOException;
-import java.net.ConnectException;
-import java.net.NoRouteToHostException;
-import java.net.SocketTimeoutException;
-
@Component
public class ServiceCombRetryExtension extends AbstractRetryExtension {
@Override
@@ -37,25 +32,21 @@ public class ServiceCombRetryExtension extends AbstractRetryExtension {
return null;
}
Response resp = (Response) result;
- if (!resp.isFailed()) {
- return null;
+ if (resp.isFailed()) {
+ if (resp.getResult() instanceof InvocationException) {
+ InvocationException e = resp.getResult();
+ return String.valueOf(e.getStatusCode());
+ }
}
- if (InvocationException.class.isInstance(resp.getResult())) {
- InvocationException e = resp.getResult();
- return String.valueOf(e.getStatusCode());
- }
- return null;
+ return String.valueOf(resp.getStatusCode());
}
@Override
- @SuppressWarnings({"unchecked", "rawtypes"})
- public Class<? extends Throwable>[] retryExceptions() {
- return new Class[] {
- ConnectException.class,
- SocketTimeoutException.class,
- IOException.class,
- VertxException.class,
- NoRouteToHostException.class,
- InvocationException.class};
+ public boolean isRetry(Throwable e) {
+ if (e instanceof InvocationException && ((InvocationException) e).getStatusCode() == Status.SERVICE_UNAVAILABLE
+ .getStatusCode()) {
+ return true;
+ }
+ return super.isRetry(e);
}
}
diff --git a/core/src/main/java/org/apache/servicecomb/core/provider/consumer/InvokerUtils.java b/core/src/main/java/org/apache/servicecomb/core/provider/consumer/InvokerUtils.java
index feb8760..773ac64 100644
--- a/core/src/main/java/org/apache/servicecomb/core/provider/consumer/InvokerUtils.java
+++ b/core/src/main/java/org/apache/servicecomb/core/provider/consumer/InvokerUtils.java
@@ -18,18 +18,12 @@
package org.apache.servicecomb.core.provider.consumer;
import static org.apache.servicecomb.core.exception.Exceptions.toConsumerResponse;
+import static org.apache.servicecomb.swagger.invocation.exception.ExceptionFactory.CONSUMER_INNER_STATUS_CODE;
-import java.io.IOException;
import java.lang.reflect.Method;
import java.lang.reflect.Type;
-import java.net.ConnectException;
-import java.net.NoRouteToHostException;
-import java.net.SocketTimeoutException;
import java.time.Duration;
-import java.util.Collections;
-import java.util.List;
import java.util.Map;
-import java.util.Map.Entry;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionStage;
import java.util.concurrent.Executors;
@@ -39,6 +33,7 @@ import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Supplier;
import javax.annotation.Nonnull;
+import javax.ws.rs.core.Response.Status;
import org.apache.servicecomb.core.Invocation;
import org.apache.servicecomb.core.SCBEngine;
@@ -54,6 +49,7 @@ import org.apache.servicecomb.core.invocation.InvocationFactory;
import org.apache.servicecomb.foundation.common.utils.AsyncUtils;
import org.apache.servicecomb.foundation.common.utils.BeanUtils;
import org.apache.servicecomb.governance.handler.RetryHandler;
+import org.apache.servicecomb.governance.handler.ext.RetryExtension;
import org.apache.servicecomb.governance.marker.GovernanceRequest;
import org.apache.servicecomb.swagger.invocation.AsyncResponse;
import org.apache.servicecomb.swagger.invocation.Response;
@@ -63,7 +59,7 @@ import org.apache.servicecomb.swagger.invocation.exception.InvocationException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import com.google.common.collect.ImmutableMap;
+import com.google.common.annotations.VisibleForTesting;
import com.netflix.config.DynamicPropertyFactory;
import io.github.resilience4j.decorators.Decorators;
@@ -74,23 +70,10 @@ import io.github.resilience4j.retry.RetryRegistry;
import io.vavr.CheckedFunction0;
import io.vavr.control.Try;
import io.vertx.core.Context;
-import io.vertx.core.VertxException;
public final class InvokerUtils {
private static final Logger LOGGER = LoggerFactory.getLogger(InvokerUtils.class);
- private static final Map<Class<? extends Throwable>, List<String>> STRICT_RETRIABLE =
- ImmutableMap.<Class<? extends Throwable>, List<String>>builder()
- .put(ConnectException.class, Collections.emptyList())
- .put(SocketTimeoutException.class, Collections.emptyList())
- /*
- * deal with some special exceptions caused by the server side close the connection
- */
- .put(IOException.class, Collections.singletonList("Connection reset by peer"))
- .put(VertxException.class, Collections.singletonList("Connection was closed"))
- .put(NoRouteToHostException.class, Collections.emptyList())
- .build();
-
private static final Object LOCK = new Object();
private static ScheduledExecutorService reactiveRetryPool;
@@ -220,37 +203,46 @@ public final class InvokerUtils {
}
}
- private static Response innerSyncInvokeImpl(Invocation invocation) {
- try {
- if (ENABLE_EVENT_LOOP_BLOCKING_CALL_CHECK && isInEventLoop()) {
- throw new IllegalStateException("Can not execute sync logic in event loop. ");
- }
- updateRetryStatus(invocation);
- SyncResponseExecutor respExecutor = new SyncResponseExecutor();
- invocation.setResponseExecutor(respExecutor);
+ private static Response innerSyncInvokeImpl(Invocation invocation) throws Throwable {
+ if (ENABLE_EVENT_LOOP_BLOCKING_CALL_CHECK && isInEventLoop()) {
+ throw new IllegalStateException("Can not execute sync logic in event loop. ");
+ }
+ updateRetryStatus(invocation);
+ SyncResponseExecutor respExecutor = new SyncResponseExecutor();
+ invocation.setResponseExecutor(respExecutor);
- invocation.onStartHandlersRequest();
- invocation.next(respExecutor::setResponse);
+ invocation.onStartHandlersRequest();
+ invocation.next(respExecutor::setResponse);
- Response response = respExecutor.waitResponse(invocation);
- invocation.getInvocationStageTrace().finishHandlersResponse();
- invocation.onFinish(response);
- return response;
- } catch (Throwable e) {
- String msg =
- String.format("invoke failed, %s", invocation.getOperationMeta().getMicroserviceQualifiedName());
- LOGGER.error(msg, e);
+ Response response = respExecutor.waitResponse(invocation);
+ if (response.isFailed()) {
+ // re-throw exception to make sure retry based on exception
+ // for InvocationException, users can configure status code for retry
+ // for 490, details error are wrapped, need re-throw
- Response response = Response.createConsumerFail(e);
- invocation.onFinish(response);
- return response;
+ if (!(response.getResult() instanceof InvocationException)) {
+ throw (Throwable) response.getResult();
+ }
+
+ if (((InvocationException) response.getResult()).getStatusCode() == CONSUMER_INNER_STATUS_CODE) {
+ throw (Throwable) response.getResult();
+ }
}
+
+ invocation.getInvocationStageTrace().finishHandlersResponse();
+ invocation.onFinish(response);
+ return response;
}
private static void updateRetryStatus(Invocation invocation) {
if (invocation.getHandlerIndex() != 0) {
// for retry, reset index
invocation.setHandlerIndex(0);
+ if (invocation.getLocalContext(RetryContext.RETRY_LOAD_BALANCE) != null
+ && (boolean) invocation.getLocalContext(RetryContext.RETRY_LOAD_BALANCE)) {
+ // clear last server to avoid using user defined endpoint
+ invocation.setEndpoint(null);
+ }
RetryContext retryContext = invocation.getLocalContext(RetryContext.RETRY_CONTEXT);
retryContext.incrementRetry();
return;
@@ -261,23 +253,36 @@ public final class InvokerUtils {
}
private static Response decorateSyncRetry(Invocation invocation, GovernanceRequest request) {
- // governance implementations.
- RetryHandler retryHandler = BeanUtils.getBean(RetryHandler.class);
- Retry retry = retryHandler.getActuator(request);
- if (retry != null) {
- CheckedFunction0<Response> supplier = Retry.decorateCheckedSupplier(retry, () -> innerSyncInvokeImpl(invocation));
- return Try.of(supplier).get();
- }
+ try {
+ // governance implementations.
+ RetryHandler retryHandler = BeanUtils.getBean(RetryHandler.class);
+ Retry retry = retryHandler.getActuator(request);
+ if (retry != null) {
+ CheckedFunction0<Response> supplier = Retry
+ .decorateCheckedSupplier(retry, () -> innerSyncInvokeImpl(invocation));
+ return Try.of(supplier).get();
+ }
- if (isCompatibleRetryEnabled(invocation)) {
- // compatible implementation for retry in load balance module in old versions.
- retry = getOrCreateCompatibleRetry(invocation);
- CheckedFunction0<Response> supplier = Retry.decorateCheckedSupplier(retry, () -> innerSyncInvokeImpl(invocation));
- return Try.of(supplier).get();
- }
+ if (isCompatibleRetryEnabled(invocation)) {
+ // compatible implementation for retry in load balance module in old versions.
+ retry = getOrCreateCompatibleRetry(invocation);
+ CheckedFunction0<Response> supplier = Retry
+ .decorateCheckedSupplier(retry, () -> innerSyncInvokeImpl(invocation));
+ return Try.of(supplier).get();
+ }
+
+ // retry not enabled
+ return innerSyncInvokeImpl(invocation);
+ } catch (Throwable e) {
+ String message = String.format("invoke failed, operation %s, trace id %s",
+ invocation.getMicroserviceQualifiedName(),
+ invocation.getTraceId());
+ LOGGER.error(message, e);
- // retry not enabled
- return innerSyncInvokeImpl(invocation);
+ Response response = Response.createConsumerFail(e, message);
+ invocation.onFinish(response);
+ return response;
+ }
}
private static boolean isCompatibleRetryEnabled(Invocation invocation) {
@@ -319,12 +324,21 @@ public final class InvokerUtils {
}
dcs.get().whenComplete((r, e) -> {
+ invocation.getInvocationStageTrace().finishHandlersResponse();
+
if (e == null) {
+ invocation.onFinish(r);
asyncResp.complete(r);
return;
}
- asyncResp.consumerFail(e);
+ String message = String.format("invoke failed, operation %s, trace id %s",
+ invocation.getMicroserviceQualifiedName(),
+ invocation.getTraceId());
+ LOGGER.error(message, e);
+ Response response = Response.createConsumerFail(e, message);
+ invocation.onFinish(response);
+ asyncResp.complete(response);
});
}
@@ -356,20 +370,29 @@ public final class InvokerUtils {
invocation.next(ar -> {
ContextUtils.setInvocationContext(invocation.getParentContext());
try {
- invocation.getInvocationStageTrace().finishHandlersResponse();
- invocation.onFinish(ar);
+ if (ar.isFailed()) {
+ // re-throw exception to make sure retry based on exception
+ // for InvocationException, users can configure status code for retry
+ // for 490, details error are wrapped, need re-throw
+
+ if (!(ar.getResult() instanceof InvocationException)) {
+ result.completeExceptionally(ar.getResult());
+ return;
+ }
+
+ if (((InvocationException) ar.getResult()).getStatusCode() == CONSUMER_INNER_STATUS_CODE) {
+ result.completeExceptionally(ar.getResult());
+ return;
+ }
+ }
+
result.complete(ar);
} finally {
ContextUtils.removeInvocationContext();
}
});
} catch (Throwable e) {
- invocation.getInvocationStageTrace().finishHandlersResponse();
- //if throw exception,we can use 500 for status code ?
- Response response = Response.createConsumerFail(e);
- invocation.onFinish(response);
- LOGGER.error("invoke failed, {}", invocation.getOperationMeta().getMicroserviceQualifiedName());
- result.complete(response);
+ result.completeExceptionally(e);
}
return result;
};
@@ -436,7 +459,17 @@ public final class InvokerUtils {
invocation.onFinish(response);
}
- private static boolean canRetryForStatusCode(Object response) {
+ @VisibleForTesting
+ static boolean canRetryForException(Throwable e) {
+ if (e instanceof InvocationException && ((InvocationException) e).getStatusCode() == Status.SERVICE_UNAVAILABLE
+ .getStatusCode()) {
+ return true;
+ }
+ return RetryExtension.canRetryForException(RetryExtension.STRICT_RETRIABLE, e);
+ }
+
+ @VisibleForTesting
+ static boolean canRetryForStatusCode(Object response) {
// retry on status code 503
if (!(response instanceof Response)) {
return false;
@@ -451,29 +484,4 @@ public final class InvokerUtils {
}
return false;
}
-
- private static boolean canRetryForException(Throwable throwableToSearchIn) {
- // retry on exception type on message match
- int infiniteLoopPreventionCounter = 10;
- while (throwableToSearchIn != null && infiniteLoopPreventionCounter > 0) {
- infiniteLoopPreventionCounter--;
- for (Entry<Class<? extends Throwable>, List<String>> c : STRICT_RETRIABLE.entrySet()) {
- Class<? extends Throwable> key = c.getKey();
- if (key.isAssignableFrom(throwableToSearchIn.getClass())) {
- if (c.getValue() == null || c.getValue().isEmpty()) {
- return true;
- } else {
- String msg = throwableToSearchIn.getMessage();
- for (String val : c.getValue()) {
- if (val.equals(msg)) {
- return true;
- }
- }
- }
- }
- }
- throwableToSearchIn = throwableToSearchIn.getCause();
- }
- return false;
- }
}
diff --git a/core/src/test/java/org/apache/servicecomb/core/provider/consumer/TestInvokerUtils.java b/core/src/test/java/org/apache/servicecomb/core/provider/consumer/TestInvokerUtils.java
index 10c56e1..409fa7f 100644
--- a/core/src/test/java/org/apache/servicecomb/core/provider/consumer/TestInvokerUtils.java
+++ b/core/src/test/java/org/apache/servicecomb/core/provider/consumer/TestInvokerUtils.java
@@ -17,197 +17,16 @@
package org.apache.servicecomb.core.provider.consumer;
+import org.apache.servicecomb.swagger.invocation.Response;
+import org.apache.servicecomb.swagger.invocation.exception.InvocationException;
+import org.junit.Assert;
+import org.junit.Test;
+
public class TestInvokerUtils {
-// @Rule
-// public ExpectedException expectedException = ExpectedException.none();
-//
-// @Mocked
-// ReferenceConfig referenceConfig;
-//
-// @Mocked
-// SchemaMeta schemaMeta;
-//
-// @Mocked
-// OperationMeta operationMeta;
-//
-// Invocation invocation;
-//
-// static Object invokeResult;
-//
-// SCBEngine scbEngine = new SCBEngine();
-//
-// static long nanoTime = 1;
-//
-// @BeforeClass
-// public static void classSetup() {
-// new MockUp<System>() {
-// @Mock
-// long nanoTime() {
-// return nanoTime;
-// }
-// };
-// }
-//
-// @Before
-// public void setup() {
-// new MockUp<SCBEngine>() {
-// @Mock
-// SCBEngine getInstance() {
-// return scbEngine;
-// }
-// };
-// scbEngine.setStatus(SCBStatus.UP);
-//
-// new Expectations() {
-// {
-// operationMeta.getSchemaMeta();
-// result = schemaMeta;
-// schemaMeta.getConsumerHandlerChain();
-// result = Arrays.asList((Handler) (i, ar) -> {
-// System.out.println(invokeResult);
-// ar.success(invokeResult);
-// });
-// }
-// };
-// invocation = new Invocation(referenceConfig, operationMeta, new Object[] {});
-// }
-//
-// @Test
-// public void testSyncInvokeInvocationWithException() {
-// Invocation invocation = Mockito.mock(Invocation.class);
-// InvocationStageTrace stageTrace = new InvocationStageTrace(invocation);
-// Mockito.when(invocation.getInvocationStageTrace()).thenReturn(stageTrace);
-//
-// Response response = Mockito.mock(Response.class);
-// new MockUp<SyncResponseExecutor>() {
-// @Mock
-// public Response waitResponse() {
-// return response;
-// }
-// };
-// Mockito.when(response.isSuccessed()).thenReturn(false);
-// OperationMeta operationMeta = Mockito.mock(OperationMeta.class);
-// Mockito.when(invocation.getOperationMeta()).thenReturn(operationMeta);
-// Mockito.when(operationMeta.getMicroserviceQualifiedName()).thenReturn("test");
-//
-// expectedException.expect(InvocationException.class);
-// expectedException.expect(Matchers.hasProperty("statusCode", Matchers.is(490)));
-// InvokerUtils.syncInvoke(invocation);
-// }
-//
-// @Test
-// public void testSyncInvokeNormal() {
-// invokeResult = 1;
-// Assert.assertEquals(1, (int) InvokerUtils.syncInvoke(invocation));
-// Assert.assertEquals(1, invocation.getInvocationStageTrace().getStart());
-// Assert.assertEquals(1, invocation.getInvocationStageTrace().getStartHandlersRequest());
-// Assert.assertEquals(1, invocation.getInvocationStageTrace().getFinishHandlersResponse());
-// Assert.assertEquals(1, invocation.getInvocationStageTrace().getFinish());
-// }
-//
-// @Test
-// public void testReactiveInvoke(@Mocked InvocationContext parentContext, @Mocked Response response) {
-// new MockUp<Invocation>(invocation) {
-// @Mock
-// InvocationContext getParentContext() {
-// return parentContext;
-// }
-//
-// @Mock
-// void next(AsyncResponse asyncResp) {
-// asyncResp.handle(response);
-// }
-// };
-//
-// Holder<InvocationContext> holder = new Holder<>();
-// InvokerUtils.reactiveInvoke(invocation, ar -> holder.value = ContextUtils.getInvocationContext());
-//
-// Assert.assertNull(ContextUtils.getInvocationContext());
-// Assert.assertSame(parentContext, holder.value);
-// Assert.assertEquals(1, invocation.getInvocationStageTrace().getStart());
-// Assert.assertEquals(1, invocation.getInvocationStageTrace().getStartHandlersRequest());
-// Assert.assertEquals(1, invocation.getInvocationStageTrace().getFinishHandlersResponse());
-// Assert.assertEquals(1, invocation.getInvocationStageTrace().getFinish());
-// }
-//
-// @Test
-// public void reactiveInvokeException() {
-// new MockUp<Invocation>(invocation) {
-// @Mock
-// void next(AsyncResponse asyncResp) {
-// throw new RuntimeExceptionWithoutStackTrace();
-// }
-// };
-//
-// Holder<Response> holder = new Holder<>();
-// InvokerUtils.reactiveInvoke(invocation, ar -> holder.value = ar);
-//
-// Assert.assertFalse(holder.value.isSuccessed());
-// Assert.assertEquals(1, invocation.getInvocationStageTrace().getStart());
-// Assert.assertEquals(1, invocation.getInvocationStageTrace().getStartHandlersRequest());
-// Assert.assertEquals(1, invocation.getInvocationStageTrace().getFinishHandlersResponse());
-// Assert.assertEquals(1, invocation.getInvocationStageTrace().getFinish());
-// }
-//
-// @SuppressWarnings("deprecation")
-// @Test
-// public void invoke() {
-// new MockUp<InvokerUtils>() {
-// @Mock
-// Object syncInvoke(Invocation invocation) {
-// return 1;
-// }
-// };
-//
-// Assert.assertEquals(1, InvokerUtils.invoke(null));
-// }
-//
-// @Test
-// public void testSyncInvoke_4param_NotReady() {
-// scbEngine.setStatus(SCBStatus.DOWN);
-//
-// expectedException.expect(InvocationException.class);
-// expectedException.expectMessage(
-// Matchers
-// .is("InvocationException: code=503;msg=CommonExceptionData [message=The request is rejected. Cannot process the request due to STATUS = DOWN]"));
-// InvokerUtils.syncInvoke("ms", "schemaId", "opName", null);
-// }
-//
-// @Test
-// public void testSyncInvoke_6param_NotReady() {
-// scbEngine.setStatus(SCBStatus.DOWN);
-//
-// expectedException.expect(InvocationException.class);
-// expectedException.expectMessage(
-// Matchers
-// .is("InvocationException: code=503;msg=CommonExceptionData [message=The request is rejected. Cannot process the request due to STATUS = DOWN]"));
-//
-// InvokerUtils.syncInvoke("ms", "latest", "rest", "schemaId", "opName", null);
-// }
-//
-// @Test
-// public void testSyncInvokeReady(@Injectable ConsumerProviderManager consumerProviderManager,
-// @Injectable Invocation invocation) {
-// scbEngine.setConsumerProviderManager(consumerProviderManager);
-//
-// new Expectations(InvocationFactory.class) {
-// {
-// InvocationFactory.forConsumer((ReferenceConfig) any, (SchemaMeta) any, (String) any, (Object[]) any);
-// result = invocation;
-// }
-// };
-// new Expectations(InvokerUtils.class) {
-// {
-// InvokerUtils.syncInvoke(invocation);
-// result = "ok";
-// }
-// };
-// Object result1 = InvokerUtils.syncInvoke("ms", "schemaId", "opName", null);
-// Assert.assertEquals("ok", result1);
-//
-// Object result2 = InvokerUtils.syncInvoke("ms", "latest", "rest", "schemaId", "opName", null);
-// Assert.assertEquals("ok", result2);
-//
-// CseContext.getInstance().setConsumerProviderManager(null);
-// }
+ @Test
+ public void testRetryInvocation503() {
+ InvocationException root = new InvocationException(503, "Service Unavailable", "Error");
+ boolean canRetry = InvokerUtils.canRetryForStatusCode(Response.failResp(root));
+ Assert.assertTrue(canRetry);
+ }
}
diff --git a/demo/demo-springmvc/springmvc-client/src/main/resources/microservice.yaml b/demo/demo-springmvc/springmvc-client/src/main/resources/microservice.yaml
index 550192b..a76dafd 100644
--- a/demo/demo-springmvc/springmvc-client/src/main/resources/microservice.yaml
+++ b/demo/demo-springmvc/springmvc-client/src/main/resources/microservice.yaml
@@ -22,7 +22,7 @@ service_description:
servicecomb:
service:
registry:
- address: http://127.0.0.1:9980,http://127.0.0.1:30100
+ address: http://127.0.0.1:30100
autodiscovery: true
client:
timeout:
@@ -34,13 +34,6 @@ servicecomb:
pull:
interval: 90
watch: true
- # can download config center from https://cse-bucket.obs.myhwclouds.com/LocalCSE/Local-CSE-1.0.0.zip to test dynamic config
- config:
- client:
- serverUri: http://127.0.0.1:30113
- refreshMode: 1
- refresh_interval: 5000
- refreshPort: 30114
rest:
client:
connection:
diff --git a/demo/demo-springmvc/springmvc-server/src/main/resources/microservice.yaml b/demo/demo-springmvc/springmvc-server/src/main/resources/microservice.yaml
index 5e3e9cb..11df205 100644
--- a/demo/demo-springmvc/springmvc-server/src/main/resources/microservice.yaml
+++ b/demo/demo-springmvc/springmvc-server/src/main/resources/microservice.yaml
@@ -32,7 +32,7 @@ servicecomb:
service:
registry:
registerPath: true
- address: http://127.0.0.1:30100
+ address: http://127.0.0.1:9980,http://127.0.0.1:30100
client:
timeout:
idle: 60
@@ -44,14 +44,6 @@ servicecomb:
interval: 10
watch: true
autodiscovery: true
- # can download config center from https://cse-bucket.obs.myhwclouds.com/LocalCSE/Local-CSE-1.0.0.zip to test dynamic config
- config:
- client:
- # serverUri: http://127.0.0.1:30113
- refreshMode: 0
- refresh_interval: 5000
- refreshPort: 30114
- fileSource: file1, file2
uploads:
directory: target
rest:
diff --git a/governance/src/main/java/org/apache/servicecomb/governance/handler/RetryHandler.java b/governance/src/main/java/org/apache/servicecomb/governance/handler/RetryHandler.java
index fed065e..7ecfc75 100644
--- a/governance/src/main/java/org/apache/servicecomb/governance/handler/RetryHandler.java
+++ b/governance/src/main/java/org/apache/servicecomb/governance/handler/RetryHandler.java
@@ -17,10 +17,6 @@
package org.apache.servicecomb.governance.handler;
import java.time.Duration;
-import java.util.List;
-import java.util.function.Predicate;
-
-import io.github.resilience4j.core.IntervalFunction;
import org.apache.servicecomb.governance.handler.ext.RetryExtension;
import org.apache.servicecomb.governance.marker.GovernanceRequest;
@@ -32,6 +28,7 @@ import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;
+import io.github.resilience4j.core.IntervalFunction;
import io.github.resilience4j.retry.Retry;
import io.github.resilience4j.retry.RetryConfig;
import io.github.resilience4j.retry.RetryRegistry;
@@ -67,8 +64,8 @@ public class RetryHandler extends AbstractGovernanceHandler<Retry, RetryPolicy>
RetryConfig config = RetryConfig.custom()
.maxAttempts(retryPolicy.getMaxAttempts() + 1)
- .retryOnResult(getPredicate(retryPolicy.getRetryOnResponseStatus()))
- .retryExceptions(retryExtension.retryExceptions())
+ .retryOnResult(response -> retryExtension.isRetry(retryPolicy.getRetryOnResponseStatus(), response))
+ .retryOnException(e -> retryExtension.isRetry(e))
.intervalFunction(getIntervalFunction(retryPolicy))
.build();
@@ -83,8 +80,4 @@ public class RetryHandler extends AbstractGovernanceHandler<Retry, RetryPolicy>
}
return IntervalFunction.of(Duration.parse(retryPolicy.getWaitDuration()));
}
-
- private Predicate<Object> getPredicate(List<String> statusList) {
- return response -> retryExtension.isRetry(statusList, response);
- }
}
diff --git a/governance/src/main/java/org/apache/servicecomb/governance/handler/ext/RetryExtension.java b/governance/src/main/java/org/apache/servicecomb/governance/handler/ext/RetryExtension.java
index c7cd145..9727aa1 100644
--- a/governance/src/main/java/org/apache/servicecomb/governance/handler/ext/RetryExtension.java
+++ b/governance/src/main/java/org/apache/servicecomb/governance/handler/ext/RetryExtension.java
@@ -17,11 +17,61 @@
package org.apache.servicecomb.governance.handler.ext;
+import java.io.IOException;
+import java.net.ConnectException;
+import java.net.NoRouteToHostException;
+import java.net.SocketTimeoutException;
+import java.util.Collections;
import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+
+import com.google.common.collect.ImmutableMap;
+
+import io.vertx.core.VertxException;
public interface RetryExtension {
+ Map<Class<? extends Throwable>, List<String>> STRICT_RETRIABLE =
+ ImmutableMap.<Class<? extends Throwable>, List<String>>builder()
+ .put(ConnectException.class, Collections.emptyList())
+ .put(SocketTimeoutException.class, Collections.emptyList())
+ /*
+ * deal with some special exceptions caused by the server side close the connection
+ */
+ .put(IOException.class, Collections.singletonList("Connection reset by peer"))
+ .put(VertxException.class, Collections.singletonList("Connection was closed"))
+ .put(NoRouteToHostException.class, Collections.emptyList())
+ .build();
+
boolean isRetry(List<String> statusList, Object result);
- Class<? extends Throwable>[] retryExceptions();
+ default boolean isRetry(Throwable e) {
+ return canRetryForException(STRICT_RETRIABLE, e);
+ }
+ static boolean canRetryForException(Map<Class<? extends Throwable>, List<String>> retryList,
+ Throwable throwableToSearchIn) {
+ // retry on exception type on message match
+ int infiniteLoopPreventionCounter = 10;
+ while (throwableToSearchIn != null && infiniteLoopPreventionCounter > 0) {
+ infiniteLoopPreventionCounter--;
+ for (Entry<Class<? extends Throwable>, List<String>> c : retryList.entrySet()) {
+ Class<? extends Throwable> key = c.getKey();
+ if (key.isAssignableFrom(throwableToSearchIn.getClass())) {
+ if (c.getValue() == null || c.getValue().isEmpty()) {
+ return true;
+ } else {
+ String msg = throwableToSearchIn.getMessage();
+ for (String val : c.getValue()) {
+ if (val.equals(msg)) {
+ return true;
+ }
+ }
+ }
+ }
+ }
+ throwableToSearchIn = throwableToSearchIn.getCause();
+ }
+ return false;
+ }
}
diff --git a/governance/src/test/java/org/apache/servicecomb/governance/MockRetryExtension.java b/governance/src/test/java/org/apache/servicecomb/governance/MockRetryExtension.java
index 50e8fc4..1ca9b4d 100644
--- a/governance/src/test/java/org/apache/servicecomb/governance/MockRetryExtension.java
+++ b/governance/src/test/java/org/apache/servicecomb/governance/MockRetryExtension.java
@@ -19,9 +19,8 @@ package org.apache.servicecomb.governance;
import java.util.List;
-import org.springframework.stereotype.Component;
-
import org.apache.servicecomb.governance.handler.ext.RetryExtension;
+import org.springframework.stereotype.Component;
@Component
public class MockRetryExtension implements RetryExtension {
@@ -31,7 +30,7 @@ public class MockRetryExtension implements RetryExtension {
}
@Override
- public Class<? extends Throwable>[] retryExceptions() {
- return null;
+ public boolean isRetry(Throwable e) {
+ return false;
}
}
diff --git a/governance/src/test/java/org/apache/servicecomb/governance/handler/ext/RetryExtensionTest.java b/governance/src/test/java/org/apache/servicecomb/governance/handler/ext/RetryExtensionTest.java
index 74d9880..8ceca63 100644
--- a/governance/src/test/java/org/apache/servicecomb/governance/handler/ext/RetryExtensionTest.java
+++ b/governance/src/test/java/org/apache/servicecomb/governance/handler/ext/RetryExtensionTest.java
@@ -17,6 +17,13 @@
package org.apache.servicecomb.governance.handler.ext;
+import java.io.IOException;
+import java.net.ConnectException;
+import java.net.NoRouteToHostException;
+import java.net.SocketTimeoutException;
+import java.util.Arrays;
+import java.util.List;
+
import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -24,8 +31,7 @@ import org.springframework.boot.test.context.ConfigDataApplicationContextInitial
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit4.SpringRunner;
-import java.util.Arrays;
-import java.util.List;
+import io.vertx.core.VertxException;
@RunWith(SpringRunner.class)
@ContextConfiguration(locations = "classpath:META-INF/spring/*.xml", initializers = ConfigDataApplicationContextInitializer.class)
@@ -70,4 +76,81 @@ public class RetryExtensionTest {
result = AbstractRetryExtension.statusCodeContains(statusList, "434");
Assert.assertTrue(result);
}
+
+ @Test
+ public void testRetryWithConnectionException() {
+ Exception target = new ConnectException("connection refused");
+ Exception root = new Exception(target);
+ boolean canRetry = RetryExtension.canRetryForException(RetryExtension.STRICT_RETRIABLE, root);
+ Assert.assertTrue(canRetry);
+ }
+
+ @Test
+ public void testRetryWithSocketTimeout() {
+ Exception target = new SocketTimeoutException("Read timed out");
+ Exception root = new Exception(target);
+ boolean canRetry = RetryExtension.canRetryForException(RetryExtension.STRICT_RETRIABLE, root);
+ Assert.assertTrue(canRetry);
+ }
+
+ @Test
+ public void testRetryWithIOException() {
+ Exception target = new IOException("Connection reset by peer");
+ Exception root = new Exception(target);
+ boolean canRetry = RetryExtension.canRetryForException(RetryExtension.STRICT_RETRIABLE, root);
+ Assert.assertTrue(canRetry);
+
+ target = new IOException("Target not exist");
+ root = new Exception(target);
+ canRetry = RetryExtension.canRetryForException(RetryExtension.STRICT_RETRIABLE, root);
+ Assert.assertFalse(canRetry);
+ }
+
+ @Test
+ public void testRetryVertxException() {
+ Exception target = new VertxException("Connection was closed");
+ Exception root = new Exception(target);
+ boolean canRetry = RetryExtension.canRetryForException(RetryExtension.STRICT_RETRIABLE, root);
+ Assert.assertTrue(canRetry);
+
+ target = new IOException("");
+ root = new Exception(target);
+ canRetry = RetryExtension.canRetryForException(RetryExtension.STRICT_RETRIABLE, root);
+ Assert.assertFalse(canRetry);
+ }
+
+ @Test
+ public void testRetryNoRouteToHostException() {
+ Exception target = new NoRouteToHostException("Host is unreachable");
+ Exception root = new Exception(target);
+ boolean canRetry = RetryExtension.canRetryForException(RetryExtension.STRICT_RETRIABLE, root);
+ Assert.assertTrue(canRetry);
+
+ target = new NoRouteToHostException("No route to host");
+ root = new Exception(target);
+ canRetry = RetryExtension.canRetryForException(RetryExtension.STRICT_RETRIABLE, root);
+ Assert.assertTrue(canRetry);
+ }
+
+ @Test
+ public void testRetryEqualTen() {
+ Exception target = new ConnectException("connectin refused");
+ for (int i = 0; i < 8; i++) {
+ target = new Exception("Level" + i, target);
+ }
+ Exception root = new Exception(target);
+ boolean canRetry = RetryExtension.canRetryForException(RetryExtension.STRICT_RETRIABLE, root);
+ Assert.assertTrue(canRetry);
+ }
+
+ @Test
+ public void testRetryOverTen() {
+ Exception target = new ConnectException("connectin refused");
+ for (int i = 0; i < 9; i++) {
+ target = new Exception("Level" + i, target);
+ }
+ Exception root = new Exception(target);
+ boolean canRetry = RetryExtension.canRetryForException(RetryExtension.STRICT_RETRIABLE, root);
+ Assert.assertFalse(canRetry);
+ }
}
diff --git a/handlers/handler-bizkeeper/src/main/java/org/apache/servicecomb/bizkeeper/BizkeeperCommand.java b/handlers/handler-bizkeeper/src/main/java/org/apache/servicecomb/bizkeeper/BizkeeperCommand.java
index 83818c8..882dcfd 100644
--- a/handlers/handler-bizkeeper/src/main/java/org/apache/servicecomb/bizkeeper/BizkeeperCommand.java
+++ b/handlers/handler-bizkeeper/src/main/java/org/apache/servicecomb/bizkeeper/BizkeeperCommand.java
@@ -85,8 +85,9 @@ public abstract class BizkeeperCommand extends HystrixObservableCommand<Response
try {
invocation.next(resp -> {
if (isFailedResponse(resp)) {
- // e should implements toString
- LOG.warn("bizkeeper command {} failed due to {}", invocation.getInvocationQualifiedName(),
+ LOG.warn("bizkeeper command {} failed, trace id {}",
+ invocation.getInvocationQualifiedName(),
+ invocation.getTraceId(),
resp.getResult());
f.onError(resp.getResult());
FallbackPolicyManager.record(type, invocation, resp, false);
@@ -97,7 +98,9 @@ public abstract class BizkeeperCommand extends HystrixObservableCommand<Response
}
});
} catch (Exception e) {
- LOG.warn("bizkeeper command {} execute failed due to {}", invocation.getInvocationQualifiedName(),
+ LOG.warn("bizkeeper command {} execute failed, trace id {}, cause {}",
+ invocation.getInvocationQualifiedName(),
+ invocation.getTraceId(),
e.getClass().getName());
f.onError(e);
}
diff --git a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadbalanceHandler.java b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadbalanceHandler.java
index dc8944f..71c7df4 100644
--- a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadbalanceHandler.java
+++ b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadbalanceHandler.java
@@ -115,8 +115,10 @@ public class LoadbalanceHandler implements Handler {
};
if (handleSuppliedEndpoint(invocation, asyncResp)) {
+ invocation.addLocalContext(RetryContext.RETRY_LOAD_BALANCE, false);
return;
}
+ invocation.addLocalContext(RetryContext.RETRY_LOAD_BALANCE, true);
String strategy = Configuration.INSTANCE.getRuleStrategyName(invocation.getMicroserviceName());
if (!Objects.equals(strategy, this.strategy)) {
@@ -198,9 +200,6 @@ public class LoadbalanceHandler implements Handler {
chosenLB.getLoadBalancerStats().incrementActiveRequestsCount(server);
ServiceCombLoadBalancerStats.INSTANCE.markSuccess(server);
}
- // clear endpoint after invocation finished. In retry, will choose a new server, this is different than
- // user defined endpoint
- invocation.setEndpoint(null);
asyncResp.handle(resp);
});
}
@@ -232,9 +231,10 @@ public class LoadbalanceHandler implements Handler {
}
}
- LOGGER.info("retry to instance [{}], last instance [{}], trace id {}",
- nextServer.getHostPort(),
- lastServer.getHostPort(),
+ LOGGER.info("operation failed {}, retry to instance [{}], last instance [{}], trace id {}",
+ invocation.getMicroserviceQualifiedName(),
+ nextServer == null ? "" : nextServer.getHostPort(),
+ lastServer == null ? "" : lastServer.getHostPort(),
invocation.getTraceId());
invocation.addLocalContext(CONTEXT_KEY_LAST_SERVER, lastServer);
return lastServer;
diff --git a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/ServiceCombServer.java b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/ServiceCombServer.java
index e9d1375..ed89cb8 100644
--- a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/ServiceCombServer.java
+++ b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/ServiceCombServer.java
@@ -20,6 +20,7 @@ package org.apache.servicecomb.loadbalance;
import java.net.URI;
import java.net.URISyntaxException;
+import org.apache.commons.lang3.StringUtils;
import org.apache.servicecomb.core.Endpoint;
import org.apache.servicecomb.core.Transport;
import org.apache.servicecomb.registry.api.registry.MicroserviceInstance;
@@ -99,15 +100,17 @@ public class ServiceCombServer extends Server {
return endpoint.getEndpoint();
}
- // take endpoints that belongs to same instance as same server
+ @Override
public boolean equals(Object o) {
if (o instanceof ServiceCombServer) {
- return this.instance.getInstanceId().equals(((ServiceCombServer) o).instance.getInstanceId());
+ return this.instance.getInstanceId().equals(((ServiceCombServer) o).instance.getInstanceId())
+ && StringUtils.equals(endpoint.getEndpoint(), ((ServiceCombServer) o).getEndpoint().getEndpoint());
} else {
return false;
}
}
+ @Override
public int hashCode() {
return this.instance.getInstanceId().hashCode();
}
diff --git a/swagger/swagger-invocation/invocation-core/src/main/java/org/apache/servicecomb/swagger/invocation/Response.java b/swagger/swagger-invocation/invocation-core/src/main/java/org/apache/servicecomb/swagger/invocation/Response.java
index 7073c96..9544bc7 100644
--- a/swagger/swagger-invocation/invocation-core/src/main/java/org/apache/servicecomb/swagger/invocation/Response.java
+++ b/swagger/swagger-invocation/invocation-core/src/main/java/org/apache/servicecomb/swagger/invocation/Response.java
@@ -179,6 +179,11 @@ public class Response {
return createFail(exception);
}
+ public static Response createConsumerFail(Throwable throwable, String message) {
+ InvocationException exception = ExceptionFactory.convertConsumerException(throwable, message);
+ return createFail(exception);
+ }
+
public static Response createProducerFail(Throwable throwable) {
InvocationException exception = ExceptionFactory.convertProducerException(throwable);
return createFail(exception);