Issue298 #339

jlaber · 2019-02-14T14:56:51Z

For this change, I didn't just allow overriding the default, I allow for shared configurations. You could have many shared configurations, or just one default. You really can do it however you want. I also removed the creation of Health Indicators from the @PostConstruct and allow for registering post circuit breaker creation handling functions. This was done so that if you create circuit breakers beyond the config file, you will still automatically get health indicators created for them.

Also:
This contains the changes from pull request: #312 - I needed the ability to override the defaults to build this functionality. I fixed the issues that I mentioned in that PR. One key note is that I did bump the spring boot 2 version to 2.1.x to allow for overriding the event consumers. If we don't want that upgrade, I can change it back, and we just won't allow that overriding until we want to upgrade to 2.1.x

- Change tests to test autoconfigure instead of scanning the entire package name which isn't the same behavior. Also has fix for handling the generic EventConsumerRegistry beans for overriding. This had to bump up the spring boot 2 version to 2.1.x. For spring boot 1.x, it doesn't allow for overriding. We can adjust this if we don't want to upgrade.

…e default" (resilience4j#298)

coveralls · 2019-02-14T18:04:41Z

Coverage remained the same at 100.0% when pulling 3fa500f on paychex:issue298 into fe3bf59 on resilience4j:master.

# Conflicts: # resilience4j-spring/src/main/java/io/github/resilience4j/circuitbreaker/configure/CircuitBreakerConfigurationProperties.java

Romeh · 2019-03-14T13:54:40Z

@jlaber can you please resolve the merge conflicts so we can play with ur PR ;)

# Conflicts: # resilience4j-spring-boot/src/test/resources/application.yaml # resilience4j-spring-boot2/src/test/java/io/github/resilience4j/circuitbreaker/CircuitBreakerAutoConfigurationTest.java # resilience4j-spring-boot2/src/test/resources/application.yaml

jlaber · 2019-03-16T14:40:51Z

@Romeh Thanks for the notification. I just merged in the conflicts. Please let me know what you think!

RobWin · 2019-03-22T09:02:32Z

Sry, could you rebase again ;)

# Conflicts: # libraries.gradle # resilience4j-spring-boot/src/main/java/io/github/resilience4j/circuitbreaker/autoconfigure/CircuitBreakerAutoConfiguration.java # resilience4j-spring-boot/src/main/java/io/github/resilience4j/ratelimiter/autoconfigure/RateLimiterAutoConfiguration.java # resilience4j-spring-boot/src/test/java/io/github/resilience4j/circuitbreaker/CircuitBreakerAutoConfigurationTest.java # resilience4j-spring-boot/src/test/resources/application.yaml # resilience4j-spring-boot2/src/main/java/io/github/resilience4j/circuitbreaker/autoconfigure/CircuitBreakerAutoConfiguration.java # resilience4j-spring-boot2/src/main/java/io/github/resilience4j/ratelimiter/autoconfigure/RateLimiterAutoConfiguration.java # resilience4j-spring-boot2/src/test/java/io/github/resilience4j/circuitbreaker/CircuitBreakerAutoConfigurationTest.java

jlaber · 2019-03-23T15:05:24Z

@RobWin - All merged in. Thanks.

Romeh · 2019-03-26T12:40:42Z

...c/main/java/io/github/resilience4j/circuitbreaker/configure/CircuitBreakerConfiguration.java

@@ -31,23 +32,23 @@
 @Configuration
 public class CircuitBreakerConfiguration {

+	private final CircuitBreakerConfigurationProperties circuitBreakerProperties;
+	private EventConsumerRegister eventConsumerRegister;


I think EventConsumerRegister not needed to be class instance variable here

Romeh · 2019-03-26T12:42:03Z

...c/main/java/io/github/resilience4j/circuitbreaker/configure/CircuitBreakerConfiguration.java

+    	eventConsumerRegister = new EventConsumerRegister(eventConsumerRegistry);
+		circuitBreakerRegistry.registerPostCreationConsumer(
+				(circuitBreaker, config) -> eventConsumerRegister.registerEventConsumer(circuitBreaker, config));
+    }


you can use method reference eventConsumerRegister::registerEventConsumer for better readability

Romeh · 2019-03-26T12:42:42Z

...a/io/github/resilience4j/circuitbreaker/configure/CircuitBreakerConfigurationProperties.java

@@ -45,12 +48,28 @@ public void setCircuitBreakerAspectOrder(int circuitBreakerAspectOrder) {
    }

    private BackendProperties getBackendProperties(String backend) {
-        return backends.get(backend);
+    	BackendProperties properties = backends.get(backend);
+    	if(properties.getSharedConfigName() != null && !properties.getSharedConfigName().isEmpty() ) {


can we replace it with if(!StringUtils.isEmpty(backendProperties.getSharedConfigName())) { for better code readability

Romeh · 2019-03-26T12:43:20Z

...a/io/github/resilience4j/circuitbreaker/configure/CircuitBreakerConfigurationProperties.java

+    public CircuitBreakerConfig createCircuitBreakerConfigFromShared(String sharedConfig) {
+        BackendProperties backendProperties = getSharedConfigProperties(sharedConfig);
+        if(backendProperties.getSharedConfigName() == null) {
+            return buildCircuitBreakerConfig(backendProperties).configurationName(sharedConfig).build();


same remark as above

Romeh · 2019-03-26T12:44:28Z

...thub/resilience4j/circuitbreaker/autoconfigure/CircuitBreakerConfigurationOnMissingBean.java

+        // Register a consumer to hook up any health indicators for circuit breakers after creation. This will catch ones that get
+        // created beyond initially configured backends.
+        circuitBreakerRegistry.registerPostCreationConsumer((circuitBreaker, config) -> createHeathIndicatorForCircuitBreaker(circuitBreaker, config));
+


can we use method reference here this::createHeathIndicatorForCircuitBreaker

Romeh · 2019-03-26T12:45:18Z

...thub/resilience4j/circuitbreaker/autoconfigure/CircuitBreakerConfigurationOnMissingBean.java

+        circuitBreakerConfiguration.registerPostCreationEventConsumer(circuitBreakerRegistry, eventConsumerRegistry);
+        // Register a consumer to hook up any health indicators for circuit breakers after creation. This will catch ones that get
+        // created beyond initially configured backends.
+        circuitBreakerRegistry.registerPostCreationConsumer((circuitBreaker, config) -> createHeathIndicatorForCircuitBreaker(circuitBreaker, config));


can we use method reference here this::createHeathIndicatorForCircuitBreaker

Romeh · 2019-03-26T12:47:58Z

...c/main/java/io/github/resilience4j/circuitbreaker/configure/CircuitBreakerConfiguration.java

+     * @param eventConsumerRegistry The event consumer registry.
+     */
+    public void initializeBackends(CircuitBreakerRegistry circuitBreakerRegistry,
+    							   EventConsumerRegistry<CircuitBreakerEvent> eventConsumerRegistry) {


EventConsumerRegistry eventConsumerRegistry is unneeded dependency here

Romeh · 2019-03-26T12:52:04Z

@jlaber i had small code comments there , can you please update them plus can u please add in spring boot 1 doc that we are not overriding Event consumer and finally add a some documentation to rate limiter that spring beans can be overridden there as well :) .

@RobWin are we going to base our-self on spring boot 2.1.* for spring boot 2 starter ?

RobWin · 2019-03-27T08:34:18Z

@Romeh I already updated our master to use the latest Spring Boot 2.1.3.RELEASE and latest Spring 1.5.19.RELEASE

…issing documentation

jlaber · 2019-03-27T15:56:15Z

@Romeh or @RobWin Are either of you familiar with that Spring Boot Asnyc Rety test that i have failing:
io.github.resilience4j.retry.RetryAutoConfigurationTest > testRetryAutoConfigurationAsync FAILED

I can't reproduce that at all when I run the tests. Does it have something to do with the Spring Context not cleaning up for it appropriately? I see the Spring Boot 2 version has a @DirtiesContext which isn't available for the Spring Boot 1.

RobWin · 2019-03-27T15:57:13Z

Yes I can fix it tomorrow

Romeh · 2019-03-29T10:46:46Z

@jlaber the test is fixed now on master , please re-base ur PR and also resolve the merge conflicts then i guess we will be good to GO :)

# Conflicts: # resilience4j-micrometer/src/main/java/io/github/resilience4j/micrometer/tagged/TaggedAsyncRetryMetrics.java # resilience4j-spring-boot/src/main/java/io/github/resilience4j/circuitbreaker/autoconfigure/CircuitBreakerPrometheusAutoConfiguration.java # resilience4j-spring-boot/src/main/java/io/github/resilience4j/ratelimiter/autoconfigure/RateLimiterPrometheusAutoConfiguration.java # resilience4j-spring-boot2/src/main/java/io/github/resilience4j/circuitbreaker/autoconfigure/CircuitBreakerMetricsAutoConfiguration.java # resilience4j-spring-boot2/src/main/java/io/github/resilience4j/ratelimiter/autoconfigure/RateLimiterMetricsAutoConfiguration.java # resilience4j-spring/src/main/java/io/github/resilience4j/retry/configure/AsyncRetryAspect.java

jlaber · 2019-04-02T02:41:25Z

@Romeh All merged in.

# Conflicts: # resilience4j-spring-boot2/src/test/java/io/github/resilience4j/circuitbreaker/CircuitBreakerAutoConfigurationTest.java # resilience4j-spring/src/main/java/io/github/resilience4j/circuitbreaker/configure/CircuitBreakerAspect.java # resilience4j-spring/src/main/java/io/github/resilience4j/circuitbreaker/configure/CircuitBreakerConfiguration.java

# Conflicts: # resilience4j-spring/src/main/java/io/github/resilience4j/ratelimiter/configure/RateLimiterConfiguration.java

# Conflicts: # resilience4j-spring-boot/src/main/java/io/github/resilience4j/circuitbreaker/autoconfigure/CircuitBreakerMetricsAutoConfiguration.java # resilience4j-spring-boot/src/main/java/io/github/resilience4j/ratelimiter/autoconfigure/RateLimiterMetricsAutoConfiguration.java

jlaber · 2019-04-04T13:39:51Z

@Romeh @RobWin
The build is failing again on one of those Retry tests:

io.github.resilience4j.retry.RetryAutoConfigurationTest > testRetryAutoConfigurationAsync FAILED
org.junit.ComparisonFailure at RetryAutoConfigurationTest.java:146
2019-04-04 13:26:23.702 INFO 6356 --- [ Thread-108] o.s.s.concurrent.ThreadPoolTaskExecutor : Shutting down ExecutorService 'applicationTaskExecutor'

I seem to only see these out on the build server and seem to be sporadic. Locally these errors don't occur.

Also, I was just wondering if you think this is a good PR in general and functionality is good? If so, would you please consider pulling the PR in. Because of the number of changes in the configuration files, it can be tough to keep up-to-date with the merge conflicts that come in from all the other new functionality. I don't want to accidentally break anything new because of missing something trying to resolve the merge.

RobWin · 2019-04-04T13:45:38Z

Yes, we are working again on fixing this test.
We will first release 0.14.0 and then take us time to merge this PR.

jlaber · 2019-04-04T13:47:33Z

Ok, cool. Thanks for the info.

jlaber · 2019-05-01T20:48:34Z

Hi @Romeh @RobWin,

It's great to see that this got merged in! I was just wondering what might have happened because it doesn't look like I show up as a contributor to the project?

Thanks,
Jason

RobWin · 2019-05-02T04:40:03Z

Good question, seems many people contributed commits to this PR and I always squash a PR before it is merged. Maybe Github uses the first commit to choose the contributor?

RobWin · 2019-05-02T04:46:58Z

But maybe you could update our Spring Boot documentation and I merge it so that you are also listed.

devcateu and others added 8 commits January 1, 2019 13:55

291 Allow overriding Spring Beans

7f3245a

Merge remote-tracking branch 'devcateu/issue291' into issue291

40554e4

Update .gitignore for Eclipse files

90bd015

Suppressing existing compiler warnings for unchecked.

4834a22

Added in the bump to version 2.1.x for Spring Boot 2 (resilience4j#291)

c50a555

Ability to use shared configurations which allows you to "override th…

e277a90

…e default" (resilience4j#298)

Noticed there was some bad text formatting. Removed tabs.

01c3d0c

jlaber added 2 commits February 21, 2019 12:50

Fixed all the static analysis issues.

0c65a69

Merge remote-tracking branch 'origin/master' into issue298

b458ae7

# Conflicts: # resilience4j-spring/src/main/java/io/github/resilience4j/circuitbreaker/configure/CircuitBreakerConfigurationProperties.java

Romeh reviewed Mar 26, 2019

View reviewed changes

jlaber added 3 commits March 27, 2019 09:09

Issue resilience4j#298: Fixed PR review comments - code cleanup and m…

a74797e

…issing documentation

Merge remote-tracking branch 'origin/master' into issue298

7947672

Issue resilience4j#298: Fix merged in test failures and build warnings.

f5680cc

jlaber added 3 commits April 3, 2019 21:33

Merge remote-tracking branch 'origin/master' into issue298

9550adb

# Conflicts: # resilience4j-spring/src/main/java/io/github/resilience4j/ratelimiter/configure/RateLimiterConfiguration.java

Romeh changed the base branch from master to spring-override April 16, 2019 12:15

Romeh merged commit c418742 into resilience4j:spring-override Apr 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue298 #339

Issue298 #339

jlaber commented Feb 14, 2019 •

edited

Loading

coveralls commented Feb 14, 2019 •

edited

Loading

Romeh commented Mar 14, 2019

jlaber commented Mar 16, 2019

RobWin commented Mar 22, 2019

jlaber commented Mar 23, 2019

Romeh Mar 26, 2019

Romeh Mar 26, 2019

Romeh Mar 26, 2019 •

edited

Loading

Romeh Mar 26, 2019

Romeh Mar 26, 2019

Romeh Mar 26, 2019

Romeh Mar 26, 2019

Romeh commented Mar 26, 2019

RobWin commented Mar 27, 2019 •

edited

Loading

jlaber commented Mar 27, 2019

RobWin commented Mar 27, 2019

Romeh commented Mar 29, 2019

jlaber commented Apr 2, 2019

jlaber commented Apr 4, 2019

RobWin commented Apr 4, 2019

jlaber commented Apr 4, 2019

jlaber commented May 1, 2019

RobWin commented May 2, 2019 •

edited

Loading

RobWin commented May 2, 2019

Issue298 #339

Issue298 #339

Conversation

jlaber commented Feb 14, 2019 • edited Loading

coveralls commented Feb 14, 2019 • edited Loading

Romeh commented Mar 14, 2019

jlaber commented Mar 16, 2019

RobWin commented Mar 22, 2019

jlaber commented Mar 23, 2019

Romeh Mar 26, 2019

Choose a reason for hiding this comment

Romeh Mar 26, 2019

Choose a reason for hiding this comment

Romeh Mar 26, 2019 • edited Loading

Choose a reason for hiding this comment

Romeh Mar 26, 2019

Choose a reason for hiding this comment

Romeh Mar 26, 2019

Choose a reason for hiding this comment

Romeh Mar 26, 2019

Choose a reason for hiding this comment

Romeh Mar 26, 2019

Choose a reason for hiding this comment

Romeh commented Mar 26, 2019

RobWin commented Mar 27, 2019 • edited Loading

jlaber commented Mar 27, 2019

RobWin commented Mar 27, 2019

Romeh commented Mar 29, 2019

jlaber commented Apr 2, 2019

jlaber commented Apr 4, 2019

RobWin commented Apr 4, 2019

jlaber commented Apr 4, 2019

jlaber commented May 1, 2019

RobWin commented May 2, 2019 • edited Loading

RobWin commented May 2, 2019

jlaber commented Feb 14, 2019 •

edited

Loading

coveralls commented Feb 14, 2019 •

edited

Loading

Romeh Mar 26, 2019 •

edited

Loading

RobWin commented Mar 27, 2019 •

edited

Loading

RobWin commented May 2, 2019 •

edited

Loading