spring-cloud / spring-cloud-consul

Spring Cloud Consul
http://cloud.spring.io/spring-cloud-consul/
Apache License 2.0
813 stars 541 forks source link

Consider adding retries to ConfigWatch to increase resiliency. #439

Closed terryherron closed 5 years ago

terryherron commented 6 years ago

Can retries increase the resilience of ConfigWatch? Specifically to retry on any random network failures. If this exception below is thrown and no other KV change occurs, the original KVs from the one event never get applied. In many cases, a retry attempt would help app instances achieve the desired state.

2018-06-20 22:54:59.767  WARN 10994 --- [ask-scheduler-4] o.s.cloud.consul.config.ConfigWatch      : service [c1d4e514022e6f2f] Error querying consul Key/Values for context 'config/application/'. Message: org.apache.http.conn.HttpHostConnectException: ...

Thanks all, Terry

spencergibb commented 6 years ago

so, it's on a scheduled executor right now, how different is that than retry? Not saying retry is a bad option.

terryherron commented 6 years ago

The difference, if not mistaken, is that if publish event fails there is no retry. Next time the scheduled index check happens it has not changed so things remain in a bad state. I believe adding retry logic in this section will allow things to heal in case of a 404 or connection refused. Next week at some point I'll try to reproduce the issue in a testcase. Perhaps I'm mistaken here. Thanks.

https://github.com/spring-cloud/spring-cloud-consul/blob/master/spring-cloud-consul-config/src/main/java/org/springframework/cloud/consul/config/ConfigWatch.java#L157

spring-projects-issues commented 5 years ago

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

spring-projects-issues commented 5 years ago

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.