[
https://issues.apache.org/jira/browse/SCB-180?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16315785#comment-16315785
]
ASF GitHub Bot commented on SCB-180:
------------------------------------
WillemJiang closed pull request #490: [SCB-180] Hystrixcommand setter not set
by dynamic configuration
URL: https://github.com/apache/incubator-servicecomb-java-chassis/pull/490
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git
a/handlers/handler-bizkeeper/src/main/java/io/servicecomb/bizkeeper/BizkeeperHandler.java
b/handlers/handler-bizkeeper/src/main/java/io/servicecomb/bizkeeper/BizkeeperHandler.java
index 5c3f166ab..9a2d5ca46 100644
---
a/handlers/handler-bizkeeper/src/main/java/io/servicecomb/bizkeeper/BizkeeperHandler.java
+++
b/handlers/handler-bizkeeper/src/main/java/io/servicecomb/bizkeeper/BizkeeperHandler.java
@@ -21,7 +21,6 @@
import org.slf4j.LoggerFactory;
import com.netflix.hystrix.HystrixCommandProperties;
-import com.netflix.hystrix.HystrixCommandProperties.ExecutionIsolationStrategy;
import com.netflix.hystrix.HystrixInvokable;
import com.netflix.hystrix.HystrixObservable;
import com.netflix.hystrix.strategy.HystrixPlugins;
@@ -43,12 +42,6 @@
protected final String groupname;
- private static final int WINDOW_IN_MILLISECONDS = 10000;
-
- private static final int WINDOW_BUCKETS = 10;
-
- private static final int SNAPSHOT_INTERVAL = 1000;
-
static {
try {
HystrixPlugins.getInstance().registerPropertiesStrategy(HystrixPropertiesStrategyExt.getInstance());
@@ -57,6 +50,7 @@
}
try {
HystrixPlugins.getInstance().registerCommandExecutionHook(new
HystrixCommandExecutionHook() {
+ @Override
public <T> Exception onExecutionError(HystrixInvokable<T>
commandInstance, Exception e) {
LOG.warn("bizkeeper execution error", e);
return e; //by default, just pass through
@@ -99,59 +93,5 @@ public void handle(Invocation invocation, AsyncResponse
asyncResp) throws Except
}
protected void setCommonProperties(Invocation invocation,
HystrixCommandProperties.Setter setter) {
- setter.withExecutionTimeoutInMilliseconds(Configuration.INSTANCE
- .getIsolationTimeoutInMilliseconds(groupname,
- invocation.getMicroserviceName(),
- invocation.getOperationMeta().getMicroserviceQualifiedName()))
-
.withExecutionIsolationSemaphoreMaxConcurrentRequests(Configuration.INSTANCE
- .getIsolationMaxConcurrentRequests(groupname,
- invocation.getMicroserviceName(),
- invocation.getOperationMeta()
- .getMicroserviceQualifiedName()))
- .withExecutionTimeoutEnabled(
- Configuration.INSTANCE.getIsolationTimeoutEnabled(groupname,
- invocation.getMicroserviceName(),
- invocation.getOperationMeta()
- .getMicroserviceQualifiedName()))
-
.withCircuitBreakerEnabled(Configuration.INSTANCE.isCircuitBreakerEnabled(groupname,
- invocation.getMicroserviceName(),
- invocation.getOperationMeta()
- .getMicroserviceQualifiedName()))
-
.withCircuitBreakerForceOpen(Configuration.INSTANCE.isCircuitBreakerForceOpen(groupname,
- invocation.getMicroserviceName(),
- invocation.getOperationMeta()
- .getMicroserviceQualifiedName()))
-
.withCircuitBreakerForceClosed(Configuration.INSTANCE.isCircuitBreakerForceClosed(groupname,
- invocation.getMicroserviceName(),
- invocation.getOperationMeta()
- .getMicroserviceQualifiedName()))
- .withCircuitBreakerSleepWindowInMilliseconds(
-
Configuration.INSTANCE.getCircuitBreakerSleepWindowInMilliseconds(groupname,
- invocation.getMicroserviceName(),
- invocation.getOperationMeta()
- .getMicroserviceQualifiedName()))
- .withCircuitBreakerRequestVolumeThreshold(
-
Configuration.INSTANCE.getCircuitBreakerRequestVolumeThreshold(groupname,
- invocation.getMicroserviceName(),
- invocation.getOperationMeta()
- .getMicroserviceQualifiedName()))
- .withCircuitBreakerErrorThresholdPercentage(
-
Configuration.INSTANCE.getCircuitBreakerErrorThresholdPercentage(groupname,
- invocation.getMicroserviceName(),
- invocation.getOperationMeta()
- .getMicroserviceQualifiedName()))
- .withFallbackEnabled(
- Configuration.INSTANCE.isFallbackEnabled(groupname,
- invocation.getMicroserviceName(),
- invocation.getOperationMeta().getMicroserviceQualifiedName()))
- .withExecutionIsolationStrategy(ExecutionIsolationStrategy.SEMAPHORE)
- .withMetricsRollingPercentileEnabled(false)
-
.withMetricsRollingStatisticalWindowInMilliseconds(WINDOW_IN_MILLISECONDS)
- .withMetricsRollingStatisticalWindowBuckets(WINDOW_BUCKETS)
- .withMetricsHealthSnapshotIntervalInMilliseconds(SNAPSHOT_INTERVAL)
-
.withFallbackIsolationSemaphoreMaxConcurrentRequests(Configuration.INSTANCE
- .getFallbackMaxConcurrentRequests(groupname,
- invocation.getMicroserviceName(),
- invocation.getOperationMeta().getMicroserviceQualifiedName()));
}
}
diff --git
a/handlers/handler-bizkeeper/src/main/java/io/servicecomb/bizkeeper/HystrixCommandPropertiesExt.java
b/handlers/handler-bizkeeper/src/main/java/io/servicecomb/bizkeeper/HystrixCommandPropertiesExt.java
index 0c4e36a7f..3a07c457a 100644
---
a/handlers/handler-bizkeeper/src/main/java/io/servicecomb/bizkeeper/HystrixCommandPropertiesExt.java
+++
b/handlers/handler-bizkeeper/src/main/java/io/servicecomb/bizkeeper/HystrixCommandPropertiesExt.java
@@ -47,9 +47,9 @@
// must occur before statistics matter
private static final Integer DEFAULT_CIRCUITBREAKERREQUESTVOLUMETHRESHOLD =
20;
- // default => sleepWindow: 5000 = 5 seconds that we will sleep before trying
+ // default => sleepWindow: 15000 = 15 seconds that we will sleep before
trying
// again after tripping the circuit
- private static final Integer DEFAULT_CIRCUITBREAKERSLEEPWINDOWINMILLISECONDS
= 5000;
+ private static final Integer DEFAULT_CIRCUITBREAKERSLEEPWINDOWINMILLISECONDS
= 15000;
// default => errorThresholdPercentage = 50 = if 50%+ of requests in 10
// seconds are failures or latent then we will trip the circuit
@@ -62,16 +62,16 @@
// default => ignoreErrors = false
static final Boolean DEFAULT_CIRCUITBREAKERFORCECLOSED = false;
- // default => executionTimeoutInMilliseconds: 1000 = 1 second
- private static final Integer DEFAULT_EXECUTIONTIMEOUTINMILLISECONDS = 1000;
+ // default => executionTimeoutInMilliseconds: 30000 = 30 second
+ private static final Integer DEFAULT_EXECUTIONTIMEOUTINMILLISECONDS = 30000;
- private static final Boolean DEFAULT_EXECUTIONTIMEOUTENABLED = true;
+ private static final Boolean DEFAULT_EXECUTIONTIMEOUTENABLED = false;
- private static final ExecutionIsolationStrategy DEFAULT_ISOLATIONSTRATEGY =
ExecutionIsolationStrategy.THREAD;
+ private static final ExecutionIsolationStrategy DEFAULT_ISOLATIONSTRATEGY =
ExecutionIsolationStrategy.SEMAPHORE;
private static final Boolean
DEFAULT_EXECUTIONISOLATIONTHREADINTERRUPTONTIMEOUT = true;
- private static final Boolean DEFAULT_METRICSROLLINGPERCENTILEENABLED = true;
+ private static final Boolean DEFAULT_METRICSROLLINGPERCENTILEENABLED = false;
private static final Boolean DEFAULT_REQUESTCACHEENABLED = true;
@@ -79,7 +79,7 @@
private static final Boolean DEFAULT_FALLBACKENABLED = true;
- private static final Integer
DEFAULT_EXECUTIONISOLATIONSEMAPHOREMAXCONCURRENTREQUESTS = 10;
+ private static final Integer
DEFAULT_EXECUTIONISOLATIONSEMAPHOREMAXCONCURRENTREQUESTS = 1000;
private static final Boolean DEFAULT_REQUESTLOGENABLED = true;
@@ -94,9 +94,9 @@
// default to 100 values max per bucket
private static final Integer DEFAULT_METRICSROLLINGPERCENTILEBUCKETSIZE =
100;
- // default to 500ms as max frequency between allowing snapshots of health
+ // default to 1000ms as max frequency between allowing snapshots of health
// (error percentage etc)
- private static final Integer
DEFAULT_METRICSHEALTHSNAPSHOTINTERVALINMILLISECONDS = 500;
+ private static final Integer
DEFAULT_METRICSHEALTHSNAPSHOTINTERVALINMILLISECONDS = 1000;
private static final int COMMAND_KEY_LENGTH = 3;
@@ -328,103 +328,128 @@ protected HystrixCommandPropertiesExt(HystrixCommandKey
key, HystrixCommandPrope
.build();
}
+ @Override
public HystrixProperty<Boolean> circuitBreakerEnabled() {
return circuitBreakerEnabled;
}
+ @Override
public HystrixProperty<Integer> circuitBreakerErrorThresholdPercentage() {
return circuitBreakerErrorThresholdPercentage;
}
+ @Override
public HystrixProperty<Boolean> circuitBreakerForceClosed() {
return circuitBreakerForceClosed;
}
+ @Override
public HystrixProperty<Boolean> circuitBreakerForceOpen() {
return circuitBreakerForceOpen;
}
+ @Override
public HystrixProperty<Integer> circuitBreakerRequestVolumeThreshold() {
return circuitBreakerRequestVolumeThreshold;
}
+ @Override
public HystrixProperty<Integer> circuitBreakerSleepWindowInMilliseconds() {
return circuitBreakerSleepWindowInMilliseconds;
}
+ @Override
public HystrixProperty<Integer>
executionIsolationSemaphoreMaxConcurrentRequests() {
return executionIsolationSemaphoreMaxConcurrentRequests;
}
+ @Override
public HystrixProperty<ExecutionIsolationStrategy>
executionIsolationStrategy() {
return executionIsolationStrategy;
}
+ @Override
public HystrixProperty<Boolean> executionIsolationThreadInterruptOnTimeout()
{
return executionIsolationThreadInterruptOnTimeout;
}
+ @Override
public HystrixProperty<String> executionIsolationThreadPoolKeyOverride() {
return executionIsolationThreadPoolKeyOverride;
}
+ @Override
@Deprecated // prefer {@link #executionTimeoutInMilliseconds}
public HystrixProperty<Integer>
executionIsolationThreadTimeoutInMilliseconds() {
return executionTimeoutInMilliseconds;
}
+ @Override
public HystrixProperty<Integer> executionTimeoutInMilliseconds() {
return executionIsolationThreadTimeoutInMilliseconds();
}
+ @Override
public HystrixProperty<Boolean> executionTimeoutEnabled() {
return executionTimeoutEnabled;
}
+ @Override
public HystrixProperty<Integer>
fallbackIsolationSemaphoreMaxConcurrentRequests() {
return fallbackIsolationSemaphoreMaxConcurrentRequests;
}
+ @Override
public HystrixProperty<Boolean> fallbackEnabled() {
return fallbackEnabled;
}
+ @Override
public HystrixProperty<Integer>
metricsHealthSnapshotIntervalInMilliseconds() {
return metricsHealthSnapshotIntervalInMilliseconds;
}
+ @Override
public HystrixProperty<Integer> metricsRollingPercentileBucketSize() {
return metricsRollingPercentileBucketSize;
}
+ @Override
public HystrixProperty<Boolean> metricsRollingPercentileEnabled() {
return metricsRollingPercentileEnabled;
}
+ @Override
public HystrixProperty<Integer> metricsRollingPercentileWindow() {
return metricsRollingPercentileWindowInMilliseconds;
}
+ @Override
public HystrixProperty<Integer>
metricsRollingPercentileWindowInMilliseconds() {
return metricsRollingPercentileWindowInMilliseconds;
}
+ @Override
public HystrixProperty<Integer> metricsRollingPercentileWindowBuckets() {
return metricsRollingPercentileWindowBuckets;
}
+ @Override
public HystrixProperty<Integer>
metricsRollingStatisticalWindowInMilliseconds() {
return metricsRollingStatisticalWindowInMilliseconds;
}
+ @Override
public HystrixProperty<Integer> metricsRollingStatisticalWindowBuckets() {
return metricsRollingStatisticalWindowBuckets;
}
+ @Override
public HystrixProperty<Boolean> requestCacheEnabled() {
return requestCacheEnabled;
}
+ @Override
public HystrixProperty<Boolean> requestLogEnabled() {
return requestLogEnabled;
}
diff --git
a/handlers/handler-bizkeeper/src/test/java/io/servicecomb/bizkeeper/TestBizkeeperHandler.java
b/handlers/handler-bizkeeper/src/test/java/io/servicecomb/bizkeeper/TestBizkeeperHandler.java
index a8b646d5c..7b7a39a1d 100644
---
a/handlers/handler-bizkeeper/src/test/java/io/servicecomb/bizkeeper/TestBizkeeperHandler.java
+++
b/handlers/handler-bizkeeper/src/test/java/io/servicecomb/bizkeeper/TestBizkeeperHandler.java
@@ -22,6 +22,8 @@
import org.junit.Before;
import org.junit.Test;
import org.mockito.Mockito;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
import com.netflix.hystrix.HystrixCommandProperties;
import com.netflix.hystrix.HystrixObservableCommand;
@@ -30,6 +32,7 @@
import io.servicecomb.core.Invocation;
import io.servicecomb.core.definition.OperationMeta;
import io.servicecomb.swagger.invocation.AsyncResponse;
+import io.servicecomb.swagger.invocation.InvocationType;
public class TestBizkeeperHandler extends BizkeeperHandler {
@@ -105,7 +108,7 @@ public void testSetCommonProperties() {
} catch (Exception e) {
validAssert = false;
}
- Assert.assertFalse(validAssert);
+ Assert.assertTrue(validAssert);
}
@Override
@@ -145,7 +148,62 @@ public void testHandleForceReturnnull() throws Exception {
System.setProperty("cse.fallbackpolicy.Group_Name.testHandleForceReturnnull.policy",
"returnnull");
bizkeeperHandler.handle(invocation, f -> {
Assert.assertTrue(f.isSuccessed());
- Assert.assertEquals(null, (Object) f.getResult());
+ Assert.assertNull(f.getResult());
+ });
+ }
+
+ @Test
+ public void testHandleInError() throws Exception {
+
Mockito.when(invocation.getMicroserviceName()).thenReturn("testHandleInError");
+
Mockito.when(invocation.getOperationMeta()).thenReturn(Mockito.mock(OperationMeta.class));
+ Mockito.when(invocation.getOperationMeta().getMicroserviceQualifiedName())
+ .thenReturn("testHandleInError");
+ FallbackPolicy policy = Mockito.mock(FallbackPolicy.class);
+ Mockito.when(policy.name()).thenReturn("throwException");
+
Mockito.when(policy.getFallbackResponse(Mockito.any(Invocation.class))).thenThrow(new
RuntimeException());
+ FallbackPolicyManager.addPolicy(policy);
+
System.setProperty("cse.fallbackpolicy.groupname.testHandleInError.policy",
"throwException");
+ Mockito.doAnswer(new Answer<Void>() {
+ @Override
+ public Void answer(InvocationOnMock invocation) {
+ AsyncResponse asyncRsp = invocation.getArgumentAt(0,
AsyncResponse.class);
+ asyncRsp.fail(InvocationType.CONSUMER, new
Exception("testHandleInError"));
+ return null;
+ }
+ }).when(invocation).next(Mockito.any(AsyncResponse.class));;
+ bizkeeperHandler.handle(invocation, f -> {
+ Assert.assertTrue(f.isFailed());
+ });
+ }
+
+ @Test
+ public void testHandlNextException() throws Exception {
+
Mockito.when(invocation.getMicroserviceName()).thenReturn("testHandlNextException");
+
Mockito.when(invocation.getOperationMeta()).thenReturn(Mockito.mock(OperationMeta.class));
+ Mockito.when(invocation.getOperationMeta().getMicroserviceQualifiedName())
+ .thenReturn("testHandlNextException");
+ Mockito.doThrow(new
Exception("testHandlNextException")).when(invocation).next(Mockito.any(AsyncResponse.class));
+ bizkeeperHandler.handle(invocation, f -> {
+ Assert.assertTrue(f.isFailed());
+ });
+ }
+
+ @Test
+ public void testHandleSuccess() throws Exception {
+
Mockito.when(invocation.getMicroserviceName()).thenReturn("testHandleSuccess");
+
Mockito.when(invocation.getOperationMeta()).thenReturn(Mockito.mock(OperationMeta.class));
+ Mockito.when(invocation.getOperationMeta().getMicroserviceQualifiedName())
+ .thenReturn("testHandleSuccess");
+ Mockito.doAnswer(new Answer<Void>() {
+ @Override
+ public Void answer(InvocationOnMock invocation) {
+ AsyncResponse asyncRsp = invocation.getArgumentAt(0,
AsyncResponse.class);
+ asyncRsp.success("");
+ return null;
+ }
+ }).when(invocation).next(Mockito.any(AsyncResponse.class));;
+ bizkeeperHandler.handle(invocation, f -> {
+ Assert.assertTrue(f.isSuccessed());
});
}
}
diff --git
a/handlers/handler-bizkeeper/src/test/java/io/servicecomb/bizkeeper/TestConfiguration.java
b/handlers/handler-bizkeeper/src/test/java/io/servicecomb/bizkeeper/TestConfiguration.java
index e4506140f..aba9084b4 100644
---
a/handlers/handler-bizkeeper/src/test/java/io/servicecomb/bizkeeper/TestConfiguration.java
+++
b/handlers/handler-bizkeeper/src/test/java/io/servicecomb/bizkeeper/TestConfiguration.java
@@ -54,5 +54,12 @@ public void testConfiguration() {
String str = c.getFallbackPolicyPolicy("groupname", test2, "testqualify");
// no need to give default value now
assertEquals(null, str);
+
+ assertFalse(c.isCircuitBreakerForceOpen("groupname", test2,
"testqualify"));
+ assertFalse(c.isCircuitBreakerForceClosed("groupname", test2,
"testqualify"));
+ assertEquals(15000,
c.getCircuitBreakerSleepWindowInMilliseconds("groupname", test2,
"testqualify"));
+ assertEquals(20, c.getCircuitBreakerRequestVolumeThreshold("groupname",
test2, "testqualify"));
+ assertEquals(50, c.getCircuitBreakerErrorThresholdPercentage("groupname",
test2, "testqualify"));
+ assertTrue(c.isFallbackEnabled("groupname", test2, "testqualify"));
}
}
diff --git
a/handlers/handler-bizkeeper/src/test/java/io/servicecomb/bizkeeper/TestHystrixPropertiesStrategyExt.java
b/handlers/handler-bizkeeper/src/test/java/io/servicecomb/bizkeeper/TestHystrixPropertiesStrategyExt.java
index d127906e7..1244ac1a7 100644
---
a/handlers/handler-bizkeeper/src/test/java/io/servicecomb/bizkeeper/TestHystrixPropertiesStrategyExt.java
+++
b/handlers/handler-bizkeeper/src/test/java/io/servicecomb/bizkeeper/TestHystrixPropertiesStrategyExt.java
@@ -70,13 +70,15 @@ public void testgetCommandProperties() {
Assert.assertFalse(commandPro.circuitBreakerForceClosed().get());
Assert.assertFalse(commandPro.circuitBreakerForceOpen().get());
Assert.assertEquals(Integer.valueOf(20),
commandPro.circuitBreakerRequestVolumeThreshold().get());
- Assert.assertEquals(Integer.valueOf(5000),
commandPro.circuitBreakerSleepWindowInMilliseconds().get());
- Assert.assertEquals(Integer.valueOf(10),
commandPro.executionIsolationSemaphoreMaxConcurrentRequests().get());
+ Assert.assertEquals(Integer.valueOf(15000),
commandPro.circuitBreakerSleepWindowInMilliseconds().get());
+ Assert.assertEquals(Integer.valueOf(1000),
commandPro.executionIsolationSemaphoreMaxConcurrentRequests().get());
Assert.assertTrue(commandPro.executionIsolationThreadInterruptOnTimeout().get());
Assert.assertEquals(null,
commandPro.executionIsolationThreadPoolKeyOverride().get());
- Assert.assertEquals(Integer.valueOf(1000),
commandPro.executionTimeoutInMilliseconds().get());
- Assert.assertTrue(commandPro.executionTimeoutEnabled().get());
+ Assert.assertEquals(Integer.valueOf(30000),
commandPro.executionTimeoutInMilliseconds().get());
+ Assert.assertFalse(commandPro.executionTimeoutEnabled().get());
Assert.assertEquals(Integer.valueOf(10),
commandPro.fallbackIsolationSemaphoreMaxConcurrentRequests().get());
Assert.assertTrue(commandPro.fallbackEnabled().get());
+ Assert.assertEquals(Integer.valueOf(100),
commandPro.metricsRollingPercentileBucketSize().get());
+ Assert.assertFalse(commandPro.metricsRollingPercentileEnabled().get());
}
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
> Circuitbreak forceopen always true
> ----------------------------------
>
> Key: SCB-180
> URL: https://issues.apache.org/jira/browse/SCB-180
> Project: Apache ServiceComb
> Issue Type: Bug
> Components: Java-Chassis
> Reporter: mojieshui
> Assignee: mojieshui
> Fix For: java-chassis-1.0.0-m1
>
>
> when use dynamic config with ConfigCenterConfigurationSource
> reproduce steps:
> 1. add dynamic config item
> "servicecomb.circuitBreaker.Consumer.forceOpen=true"
> 2. make any request, hystrixcommand will init and cached. the circuitbreaker
> is open.
> 3. delete dynamic config item
> "servicecomb.circuitBreaker.Consumer.forceOpen", Expected result is
> circuitbreaker change to the default value "false", but it keep always true,
> all request fail.
> in class HystrixCommandPropertiesExt
> {code}
> this.circuitBreakerForceOpen = getProperty(propertyPrefix,
> "circuitBreaker",
> key,
> "forceOpen",
> builder.getCircuitBreakerForceOpen(),
> DEFAULT_CIRCUITBREAKERFORCEOPEN);
> {code}
> and builder's value was set by dynamic config, not the default value false,
> if config was set before hystixcommand init, default value will override, and
> you have to restart if you want to restore
> {code}
>
> .withCircuitBreakerForceOpen(Configuration.INSTANCE.isCircuitBreakerForceOpen(groupname,
> invocation.getMicroserviceName(),
> invocation.getOperationMeta()
> .getMicroserviceQualifiedName()))
> {code}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
