Failsafe 3 #1416

Semernitskaya · 2023-02-22T11:37:46Z

Description

Upgrade Failsafe dependency to the latest version (3.3.0)

Motivation and Context

Task: #1394
Details: there are many breaking changes were introduced between current (2.4.3) and latest (3.3.0) Failsafe versions, including:

Packages renaming
Replacing constructors with builders (e.g. CircuitBreaker)
Renaming methods
Removing classes and interfaces (e.g. DelayFunction )

In this PR:

Code and tests are fixed based on introduced changes
Code samples in documentation are fixed
New section is added to MIGRATION.md file with migration recommendations and links for riptide-failsafe users

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.

Semernitskaya · 2023-02-28T14:46:29Z

riptide-failsafe/src/main/java/org/zalando/riptide/failsafe/RateLimitResetDelayFunction.java

                .filter(HttpResponseException.class::isInstance)
                .map(HttpResponseException.class::cast)
                .map(response -> response.getResponseHeaders().getFirst("X-RateLimit-Reset"))
                .map(parser::parse)
-                .orElse(null);
+                .orElse(Duration.ofMinutes(-1));


Changed to prevent NPE in DelayablePolicy line 52, behaviour for negative result and null result is the same at the end - see DelayablePolicy line 62

Semernitskaya · 2023-02-28T14:53:37Z

...oot-autoconfigure/src/main/java/org/zalando/riptide/autoconfigure/FailsafePluginFactory.java

    public static CircuitBreaker<ClientHttpResponse> createCircuitBreaker(
            final Client client,
            final CircuitBreakerListener listener) {

-        final CircuitBreaker<ClientHttpResponse> breaker = new CircuitBreaker<>();
+        final CircuitBreakerBuilder<ClientHttpResponse> breakerBuilder = CircuitBreaker.builder();


Now CircuitBreaker and other policies are created via builder() method and can't be changed after - this limits how developers can modify beans after creation.
We can think about supporting more Failsafe properties in our configuration

@Semernitskaya what additional properties that were introduced in failsafe can we support?

I meant not new properties, but supporting more of existing properties. Earlier developers could autowire CircuitBreaker bean for example and add some logic like handleIf(...) - now CircuitBreaker and other policies can't be changed after creation. So we can support more of their properties in our autoconfiguration instead

Semernitskaya · 2023-03-06T09:16:55Z

riptide-failsafe/src/main/java/org/zalando/riptide/failsafe/BackupRequest.java

 @Getter
 public final class BackupRequest<R> implements Policy<R> {

    private final long delay;
    private final TimeUnit unit;
+    private final PolicyConfig<R> config = new PolicyConfig<R>() {


Added for Policy<R> interface compatibility

MIGRATION.md

cberg-zalando · 2023-03-08T10:43:14Z

riptide-failsafe/README.md

@@ -11,7 +11,7 @@ and a circuit breaker to every remote call.
 ## Example

 ```java
-Http.builder()
+Http.builder().asyncRequestFactory(asyncRequestFactory)


Given the example, I wonder where we get the instance of asyncRequestFactory from or is this a Spring injectable dependency?

Fixed, also code sample fixed in resilience.md

cberg-zalando · 2023-03-08T10:45:33Z

riptide-failsafe/src/main/java/org/zalando/riptide/failsafe/BackupRequest.java

+    public BackupRequest(long delay, TimeUnit unit) {
+        this.delay = delay;
+        this.unit = unit;
+    }

    @Override
    @SuppressWarnings("unchecked")


As there is no cast done in the implementation, you should be able to remove this annotation, no?

cberg-zalando · 2023-03-08T11:10:41Z

riptide-failsafe/src/main/java/org/zalando/riptide/failsafe/CompositeDelayFunction.java

        return functions.stream()
-                .map(function -> function.computeDelay(result, failure, context))
+                .map(function -> {
+                    try {


Given that the method signature changed to throw instances of Throwable, why do we need to wrap this into a RuntimeException?

map method can't accept function that throws checked exception

riptide-failsafe/src/test/java/org/zalando/riptide/failsafe/CheckedPredicateConverterTest.java

cberg-zalando · 2023-03-08T11:14:31Z

riptide-failsafe/src/test/java/org/zalando/riptide/failsafe/CheckedPredicateConverterTest.java

+
+import java.util.function.Predicate;
+
+import static org.junit.jupiter.api.Assertions.*;


As in the other PR, I don't know in how this project typically expands * imports.

Replaced with full import similarly to other tests

cberg-zalando · 2023-03-08T11:16:43Z

...ide-failsafe/src/test/java/org/zalando/riptide/failsafe/FailsafePluginBackupRequestTest.java

@@ -12,6 +12,8 @@
 import org.zalando.riptide.httpclient.ApacheClientHttpRequestFactory;

 import java.io.IOException;
+import java.time.Duration;


Given that this imports where just added, but they are not used, the change looks unintended.

cberg-zalando · 2023-03-08T11:18:32Z

riptide-micrometer/src/test/java/org/zalando/riptide/micrometer/MicrometerPluginTest.java

@@ -57,6 +61,7 @@ final class MicrometerPluginTest {

    private final MeterRegistry registry = new SimpleMeterRegistry();

+


Extra new line is unintended, I assume.

Co-authored-by: Christoph Berg <christoph.berg@zalando.de>

cberg-zalando · 2023-03-09T06:44:37Z

riptide-failsafe/src/test/java/org/zalando/riptide/failsafe/CheckedPredicateConverterTest.java

+    }
+


Suggested change

}

}

It just deletes the superfluous newline.

cberg-zalando · 2023-03-09T06:45:12Z

riptide-micrometer/src/test/java/org/zalando/riptide/micrometer/MicrometerPluginTest.java

    void shouldRecordSuccessResponseMetric() {
+


Suggested change

void shouldRecordSuccessResponseMetric() {

void shouldRecordSuccessResponseMetric() {

Same as above.

cberg-zalando · 2023-03-09T16:10:55Z

👍

cberg-zalando · 2023-03-09T16:11:29Z

But take my 👍 with a grain of salt as I am not a Failsafe expert.

fatroom · 2023-03-10T16:50:23Z

👍

fatroom · 2023-03-10T16:50:56Z

@Semernitskaya thank you for the contribution
@cberg-zalando thank you for the review

Semernitskaya and others added 17 commits February 16, 2023 11:16

failsafe 3 - update version, package names

1716824

failsafe 3 - fix package names

ba34428

failsafe 3 - replace DelayFunction with ContextualSupplier

ff22533

failsafe 3 - fix ContextualSupplier usage

dfb5edb

failsafe 3 - not use AbstractExecution

209568c

failsafe 3 - fix tests

599f8cf

failsafe 3 - fix tests

25aff4e

failsafe 3 - fix tests

6bf111c

failsafe 3 - fix other modules

3b87deb

failsafe 3 - fix autoconfigure module

706e31d

failsafe 3 - fix TODOs

d9be48c

failsafe 3 - fix test coverage

3d52cb7

failsafe 3 - fix test coverage

ec22ef5

failsafe 3 - fix TODOs

7d8bc84

failsafe 3 - fix build

a6fc49f

failsafe 3 - fix build

46a1299

Merge branch 'main' into failsafe_3

dca2189

Semernitskaya commented Feb 28, 2023

View reviewed changes

Semernitskaya added 4 commits February 28, 2023 16:57

failsafe 3 - minor fixes

2a577f8

failsafe 3 - fix docs

d533d1d

failsafe 3 - fix docs

2da55e6

failsafe 3 - fix docs

7ff6773

Semernitskaya commented Mar 6, 2023

View reviewed changes

Semernitskaya marked this pull request as ready for review March 6, 2023 09:28

Semernitskaya requested review from whiskeysierra, lukasniemeier-zalando, dingxiangfei2009, Joyfolk and gpradeepkrishna as code owners March 6, 2023 09:28

cberg-zalando reviewed Mar 8, 2023

View reviewed changes

Semernitskaya and others added 5 commits March 8, 2023 15:50

Update MIGRATION.md

76aa240

Co-authored-by: Christoph Berg <christoph.berg@zalando.de>

failsafe 3 - fixes after CR

2bb5fe3

failsafe 3 - fixes after CR

0f506a2

failsafe 3 - fixes after CR

d5f54e6

failsafe 3 - fix build

f9c8706

cberg-zalando reviewed Mar 9, 2023

View reviewed changes

Semernitskaya added 2 commits March 9, 2023 17:09

failsafe 3 - fixes after CR

93bc51a

failsafe 3 - fixes after CR

43102e4

fatroom merged commit 2f120ad into zalando:main Mar 10, 2023

fatroom mentioned this pull request Mar 10, 2023

Switch to failsafe 3 #1394

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Failsafe 3 #1416

Failsafe 3 #1416

Semernitskaya commented Feb 22, 2023 •

edited

Semernitskaya Feb 28, 2023 •

edited

Semernitskaya Feb 28, 2023

fatroom Mar 1, 2023

Semernitskaya Mar 2, 2023

Semernitskaya Mar 6, 2023

cberg-zalando Mar 8, 2023

Semernitskaya Mar 8, 2023

cberg-zalando Mar 8, 2023

Semernitskaya Mar 8, 2023

cberg-zalando Mar 8, 2023

Semernitskaya Mar 8, 2023

cberg-zalando Mar 8, 2023

Semernitskaya Mar 8, 2023

cberg-zalando Mar 8, 2023

Semernitskaya Mar 8, 2023

cberg-zalando Mar 8, 2023

Semernitskaya Mar 8, 2023

cberg-zalando Mar 9, 2023

cberg-zalando Mar 9, 2023

Semernitskaya Mar 9, 2023

cberg-zalando Mar 9, 2023

cberg-zalando Mar 9, 2023

Semernitskaya Mar 9, 2023

cberg-zalando commented Mar 9, 2023

cberg-zalando commented Mar 9, 2023

fatroom commented Mar 10, 2023

fatroom commented Mar 10, 2023


		import java.util.function.Predicate;

		import static org.junit.jupiter.api.Assertions.*;

		@@ -57,6 +61,7 @@ final class MicrometerPluginTest {

		private final MeterRegistry registry = new SimpleMeterRegistry();

	void shouldRecordSuccessResponseMetric() {
	void shouldRecordSuccessResponseMetric() {

Failsafe 3 #1416

Failsafe 3 #1416

Conversation

Semernitskaya commented Feb 22, 2023 • edited

Description

Motivation and Context

Types of changes

Checklist:

Semernitskaya Feb 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cberg-zalando commented Mar 9, 2023

cberg-zalando commented Mar 9, 2023

fatroom commented Mar 10, 2023

fatroom commented Mar 10, 2023

Semernitskaya commented Feb 22, 2023 •

edited

Semernitskaya Feb 28, 2023 •

edited