[SCB-444]try to optimize autoDiscovery function#625
[SCB-444]try to optimize autoDiscovery function#625mt-monster wants to merge 6 commits intoapache:masterfrom mt-monster:master
Conversation
| this.retry = retry; | ||
| } | ||
|
|
||
| public int getRetryTimes() { |
There was a problem hiding this comment.
You can make this as a default retry mechanism, and remove setRetry.
| private boolean autoDiscoveryInited = false; | ||
|
|
||
| public ArrayList<IpPort> getDefaultIpPort() { | ||
| return defaultIpPort; |
There was a problem hiding this comment.
You can add a method such as getMaxRetryTimes based on available ips(not defaultIpPort), and minimum value is 2.
| String.format(Const.REGISTRY_API.MICROSERVICE_HEARTBEAT, microserviceId, microserviceInstanceId), | ||
| new RequestParam().setTimeout(ServiceRegistryConfig.INSTANCE.getHeartBeatRequestTimeout()), | ||
| syncHandler(countDownLatch, HttpClientResponse.class, holder)); | ||
| syncHandlerForHeartbeat(countDownLatch, HttpClientResponse.class, holder)); |
There was a problem hiding this comment.
Do not duplicate this code as mentioned above.
|
|
||
| public void setRetry(boolean retry) { | ||
| this.retry = retry; | ||
| public void setRetryTimes(int retryTimes) { |
There was a problem hiding this comment.
using a incrementRetryTimes makes logic simpler
| private boolean autoDiscoveryInited = false; | ||
|
|
||
| public int getMaxRetryTimes() { | ||
| return currentAvailableIndex.get() < 2 ? 2 : currentAvailableIndex.get(); |
There was a problem hiding this comment.
This logic is not correct. Assume current index is 2, and failed, index becomes 0. In this situation, only retried 2 and you expect 3 times
| public void setRetry(boolean retry) { | ||
| this.retry = retry; | ||
| public void incrementRetryTimes(int retryTimes) { | ||
| this.retryTimes = retryTimes + 1; |
There was a problem hiding this comment.
++this.retryTimes is more simple, and you do not need a input parameter
| LOGGER.warn("invoke service [{}] failed, retry.", requestContext.getUri()); | ||
| requestContext.setIpPort(ipPortManager.getNextAvailableAddress(requestContext.getIpPort())); | ||
| requestContext.setRetry(true); | ||
| requestContext.incrementRetryTimes(requestContext.getRetryTimes()); |
There was a problem hiding this comment.
See comments above, this code is quite bad, difficult to understand
| } | ||
| int initialIndex = new Random().nextInt(defaultIpPort.size()); | ||
| currentAvailableIndex = new AtomicInteger(initialIndex); | ||
| maxRetryTimes = defaultIpPort.size() + (getDiscoveredIpPort() == null ? 0 : getDiscoveredIpPort().size()); |
There was a problem hiding this comment.
This code is not correct. maxRetryTimes is dynamically change and you can not getDiscoveryIpPort when initialize IpPortManager
3 participants
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.--- @WillemJiang @wujimin @liubao68
This PR is intend to optimize autoDiscovery function. as we know ,now we use retry at the first time sdk fail to register sc . when deployed in multi sc ( more than one sc instance ) enviroment , one sc instance breakup, this stategy exhausts lot of time to switch ip to next one , so i think it better to retry all the loop-ips in enviroment so that sdk can meet the health sc intance.