[SCB-627] Client Request Timeout support for operation/schema/service level #744
Conversation
asifdxtreme
requested review from
wujimin,
WillemJiang and
liubao68
and removed request for
wujimin
…rrecting test case
| * @param config config parameter | ||
| * @return long value | ||
| */ | ||
| private static long getConfigParam(String config, long defaultValue) { |
There was a problem hiding this comment.
Maybe you can refer to https://github.com/apache/incubator-servicecomb-java-chassis/blob/master/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/Configuration.java to make this method simpler.
And you do not need to cache configure center values, since DynamicPropertyFactory.getInstance().getLongProperty will get the latest value for you.
| String cfgName = dynamicLongProperty.getName(); | ||
|
|
||
| //store the value in config center map and check for next requests. | ||
| configCenterValue.put(cfgName, new AtomicLong(newValue)); |
There was a problem hiding this comment.
we will modify to long. multiple threads will not operate on this map so atomic not required in this case.
| */ | ||
| private static long getConfigValue(String config) { | ||
| //first need to check in config center map which has high priority. | ||
| if (configCenterValue.containsKey(config)) { |
There was a problem hiding this comment.
why named "configCenterValue", it's just current value, right?
There was a problem hiding this comment.
ok. we will name accordingly
| String schema = invocation.getSchemaId(); | ||
| String serviceName = invocation.getMicroserviceName(); | ||
|
|
||
| config = CONSUMER_REQUEST_TIMEOUT + "." + serviceName + "." + schema + "." + operationName; |
There was a problem hiding this comment.
try logic for every invocation is not a good idea
it's not better than liubao's suggestion, but more complex.
There was a problem hiding this comment.
i just read and passed operation name/schema id/service name to the method now. any suggestion on how to get operation name/schema/service name without invocation object ?
| request: | ||
| timeout: | ||
| jaxrs: | ||
| clientreqtimeout: |
There was a problem hiding this comment.
how to express jaxrs timeout is x, but jaxrs.clientreqtimeout is y, and jaxrs.clientreqtimeout.add is z
below is not a valid yaml:
a: 10
b: 20
There was a problem hiding this comment.
ok. we will modify the key structure.
Follow this checklist to help us incorporate your contribution quickly and easily:
[SCB-XXX] Fixes bug in ApproximateQuantiles, where you replaceSCB-XXXwith the appropriate JIRA issue.mvn clean installto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.