add executor service param #1503

Semernitskaya · 2023-08-27T18:25:03Z

Description

Changes allow to specify custom ExecutorService for the FailsafePlugin. If ExecutorService is not specified - we don't use any default value and delegate executor creation to the Failsafe library.

It is also possible to provide custom ExecutorService as a bean if Spring Boot starter is used. Since 4 different FailsafePlugin-s can be created for each Http client (see FailsafePluginFactory) - 4 custom ExecutorService-s can be specified (naming convention is provided in Spring Boot starter documentation). If no custom ExecutorService is provided - ExecutorService creation again will be delegated to the the Failsafe library, no ExecutorService beans will be created.

Motivation and Context

Closes #1501

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.

fatroom · 2023-09-11T07:21:52Z

@Semernitskaya will you continue working on this feature? Do you need any support?

Semernitskaya · 2023-09-11T08:02:20Z

@Semernitskaya will you continue working on this feature? Do you need any support?

Hello! Yes, sorry for the delay - had several urgent tasks in the team's projects, so I'm a bit behind the schedule with this task

Semernitskaya · 2023-09-18T14:10:50Z

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

@@ -57,10 +64,14 @@ public RequestExecution aroundAsync(final RequestExecution execution) {

            if (policies.isEmpty()) {
                return execution.execute(arguments);
+            } else if (executorService != null) {
+              return Failsafe.with(select(arguments))
+                        .with(executorService)


From Failsafe doc for the with(ExecutorService) method:

Note: The {@code executorService} should have a core pool size or parallelism of at least 2 in order for {@link Timeout timeouts} to work

See the whole method description: https://github.com/failsafe-lib/failsafe/blob/master/core/src/main/java/dev/failsafe/FailsafeExecutor.java#L324

I've checked Timeout logic with single thread ExecutorService and it seems to be working - probably I've missed something in my check.
The question is - do we want to add validation for the ExecutorService parameter and throw an exception if parallelism is 1

Given that this is not considered as a configuration error, a warning message should be sufficient, I'd say.

Fixed: I've added a warning when it is possible to check corePoolSize i.e. when executorService instanceof ThreadPoolExecutor

Semernitskaya · 2023-09-18T14:33:32Z

riptide-spring-boot-autoconfigure/README.md

@@ -606,10 +606,17 @@ The following table shows all beans with their respective name (for the `example
 | `exampleFaultClassifier`          | `FaultClassifier`                             |
 | `exampleCircuitBreakerListener`   | `CircuitBreakerListener`                      |
 | `exampleAuthorizationProvider`    | `AuthorizationProvider`                       |
+| `exampleRetryPolicyExecutorService`    | `ExecutorService`                       |


It would be nice to add these beans to the beans diagram as well https://github.com/zalando/riptide/pull/1503/files#diff-cac26b0853dfe7f7df5898467d9a7d21fe3c0d656f8debcaedab57f4c6d20f39L567 - do we have an editable version of the diagram?

I assume, you are also using IntelliJ… Can you use its feature to align the table so it is properly aligned?

cberg-zalando · 2023-09-19T03:54:19Z

riptide-failsafe/README.md

-                .build()))
-        .build();
+                .build())
+        .withExecutor(Executors.newFixedThreadPool(2)))


When I read the description of this section, I am wondering why I need to specify the withExececutor part. At least my 🧠 does not connect the content of it mentioning retries and circuit breakers with anything executor related.

Agree - deleted executor part from this example, it will be shown only in the Custom executor section

riptide-failsafe/README.md

cberg-zalando · 2023-09-19T03:59:40Z

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

@@ -57,10 +64,14 @@ public RequestExecution aroundAsync(final RequestExecution execution) {

            if (policies.isEmpty()) {
                return execution.execute(arguments);
+            } else if (executorService != null) {
+              return Failsafe.with(select(arguments))
+                        .with(executorService)


Given that this is not considered as a configuration error, a warning message should be sufficient, I'd say.

cberg-zalando · 2023-09-19T04:00:44Z

...de-failsafe/src/test/java/org/zalando/riptide/failsafe/FailsafePluginCustomExecutorTest.java

+import okhttp3.mockwebserver.MockWebServer;
+import org.apache.hc.client5.http.impl.classic.CloseableHttpClient;
+import org.apache.hc.client5.http.impl.classic.HttpClientBuilder;
+import org.jetbrains.annotations.NotNull;


Shouldn't this be from javax.annotation?

cberg-zalando · 2023-09-19T04:02:15Z

...de-failsafe/src/test/java/org/zalando/riptide/failsafe/FailsafePluginCustomExecutorTest.java

+@Slf4j
+final class FailsafePluginCustomExecutorTest {
+
+    public static class CountingExecutorService extends ThreadPoolExecutor {


Consider making it private to avoid any other test class from using it?!

...de-failsafe/src/test/java/org/zalando/riptide/failsafe/FailsafePluginCustomExecutorTest.java

cberg-zalando · 2023-09-19T04:05:23Z

riptide-spring-boot-autoconfigure/README.md

@@ -606,10 +606,17 @@ The following table shows all beans with their respective name (for the `example
 | `exampleFaultClassifier`          | `FaultClassifier`                             |
 | `exampleCircuitBreakerListener`   | `CircuitBreakerListener`                      |
 | `exampleAuthorizationProvider`    | `AuthorizationProvider`                       |
+| `exampleRetryPolicyExecutorService`    | `ExecutorService`                       |


I assume, you are also using IntelliJ… Can you use its feature to align the table so it is properly aligned?

cberg-zalando · 2023-09-19T04:17:06Z

...t-autoconfigure/src/main/java/org/zalando/riptide/autoconfigure/DefaultRiptideRegistrar.java

-                                .setFactoryMethod("createCircuitBreakerPlugin")
-                                .addConstructorArgValue(registerCircuitBreaker(id, client))
-                                .addConstructorArgValue(createTaskDecorators(id, client));
+                        return executorService.map(executorServiceBeanName -> genericBeanDefinition(FailsafePluginFactory.class)


Given that registry.find gives you an optional, can't you just use map(name -> ref(name)).orElse(null) and then just pass this to the last addConstructorArg call given that you nevertheless allow to call it with null? Then you only need to call the genericBeanDefiniton once.

Good catch 🙂, fixed this as well

Co-authored-by: Christoph Berg <[email protected]>

…riptide into specify-failsafe-executor

cberg-zalando · 2023-09-19T08:14:17Z

To avoid these merges as in b3e9ebb due to different commits on local and remote side, you can next time use git fetch && git rebase --onto=origin/Semernitskaya:specify-failsafe-executor.

cberg-zalando · 2023-09-19T08:37:36Z

riptide-failsafe/README.md

@@ -113,6 +113,26 @@ Http.builder().requestFactory(new HttpComponentsClientHttpRequestFactory())
    .build();
 ```

+### Custom executor


When looking at the preview of the README file, I noticed that we still link to the failsafe-actuator project. Given that it was archived in December of last year, I don't know if we should keep this or not. @fatroom Any opinion?

This is a bit broad question, I'm considering either merge that project, or take over maintenance of it.

As we had the same issue, @nantipov fixed it by copying the class into our guniea pig project and do the necessary adjustments for Spring Boot 3.

Semernitskaya · 2023-09-20T10:14:20Z

@fatroom , @cberg-zalando
I also started to think that providing custom ExecutorService can be dangerous in some cases (e.g. user specified single thread executor and this affects performance, or user shared the same ExecutorService between several Plugins or Http clients) - should we write some warning in the documentation or we expect that if users do such customization - they know what they are doing

cberg-zalando · 2023-09-20T10:24:41Z

👍

cberg-zalando · 2023-09-20T10:25:55Z

@fatroom , @cberg-zalando I also started to think that providing custom ExecutorService can be dangerous in some cases (e.g. user specified single thread executor and this affects performance, or user shared the same ExecutorService between several Plugins or Http clients) - should we write some warning in the documentation or we expect that if users do such customization - they know what they are doing

I would assume the classical With great power comes great responsibility attitude…

fatroom · 2023-09-25T08:02:03Z

@Semernitskaya

I also started to think that providing custom ExecutorService can be dangerous in some cases (e.g. user specified single thread executor and this affects performance, or user shared the same ExecutorService between several Plugins or Http clients) - should we write some warning in the documentation or we expect that if users do such customization - they know what they are doing

Generally riptide already declaring that it's providing a 'sane' defaults, but maybe worth to put a warning related to this case (warn agains impact of single thread pool)

cberg-zalando · 2023-10-02T10:11:32Z

riptide-failsafe/README.md

@@ -133,6 +133,9 @@ Http.builder().requestFactory(new HttpComponentsClientHttpRequestFactory())
 If no executor is specified, the default executor configured by `Failsafe` is used. See [Failsafe DelegatingScheduler class](https://github.com/failsafe-lib/failsafe/blob/master/core/src/main/java/dev/failsafe/internal/util/DelegatingScheduler.java#L111), 
 and also [Failsafe documentation](https://failsafe.dev/async-execution/#executorservice-configuration) for more information.

+**Beware** when specifying a custom `ExecutorService` - the `ExecutorService` should have a core pool size or parallelism 


Didn't @fatroom also mention something about mentioning to not use the same ExecutorService instance for everything?

I can't see it, but I can add this recommendation as well

To quote from @fatroom user shared the same ExecutorService between several Plugins or Http clients.

But maybe he meant something different here…

cberg-zalando · 2023-10-02T10:49:22Z

👍

fatroom · 2023-10-05T12:43:43Z

Thank you for the contribution!

add executor service param

0e8ab48

Semernitskaya added 9 commits September 12, 2023 16:46

add executor service param - basic test

7ea63c9

add executor service param - basic test

d20b264

add executor service param - minor fixes

78dd557

add executor service param - update doc

2935239

add executor service param - update doc

5f0258b

add executor service param - spring boot autoconfigure

837b3fd

add executor service param - spring boot autoconfigure

f852584

add executor service param - spring boot autoconfigure, add docs

00aa890

add executor service param - minor fixes

908a2a4

Semernitskaya commented Sep 18, 2023

View reviewed changes

Semernitskaya marked this pull request as ready for review September 18, 2023 14:24

Semernitskaya requested review from whiskeysierra, lukasniemeier-zalando, fatroom, dingxiangfei2009, Joyfolk and gpradeepkrishna as code owners September 18, 2023 14:24

Semernitskaya commented Sep 18, 2023

View reviewed changes

cberg-zalando reviewed Sep 19, 2023

View reviewed changes

Semernitskaya and others added 4 commits September 19, 2023 09:54

Update riptide-failsafe/README.md

4354e33

Co-authored-by: Christoph Berg <[email protected]>

Update riptide-failsafe/README.md

ac618fb

Co-authored-by: Christoph Berg <[email protected]>

add executor service param - fixes after review

4283bf0

Merge branch 'specify-failsafe-executor' of github.com:Semernitskaya/…

b3e9ebb

…riptide into specify-failsafe-executor

cberg-zalando reviewed Sep 19, 2023

View reviewed changes

Semernitskaya added 2 commits September 19, 2023 17:52

add executor service param - fixes after review

4ef296d

add executor service param - fixes after review

a907bd5

Semernitskaya added 2 commits September 20, 2023 12:34

add executor service param - fixes after review

c9ff12c

add executor service param - fixes after review

2ace8eb

add executor service param - fixes after review

172c563

cberg-zalando reviewed Oct 2, 2023

View reviewed changes

add executor service param - fixes after review

8805924

fatroom merged commit 89ce17e into zalando:main Oct 5, 2023

fatroom mentioned this pull request Dec 5, 2023

Added additional tests for failsafe logging #1548

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add executor service param #1503

add executor service param #1503

Semernitskaya commented Aug 27, 2023 •

edited

Loading

fatroom commented Sep 11, 2023

Semernitskaya commented Sep 11, 2023

Semernitskaya Sep 18, 2023

cberg-zalando Sep 19, 2023

Semernitskaya Sep 19, 2023 •

edited

Loading

Semernitskaya Sep 18, 2023 •

edited

Loading

cberg-zalando Sep 19, 2023

Semernitskaya Sep 19, 2023

cberg-zalando Sep 19, 2023

Semernitskaya Sep 19, 2023

cberg-zalando Sep 19, 2023

cberg-zalando Sep 19, 2023

Semernitskaya Sep 19, 2023

cberg-zalando Sep 19, 2023

Semernitskaya Sep 19, 2023

cberg-zalando Sep 19, 2023

cberg-zalando Sep 19, 2023

Semernitskaya Sep 20, 2023

cberg-zalando commented Sep 19, 2023 •

edited

Loading

cberg-zalando Sep 19, 2023

fatroom Sep 25, 2023

cberg-zalando Sep 25, 2023

Semernitskaya commented Sep 20, 2023 •

edited

Loading

cberg-zalando commented Sep 20, 2023

cberg-zalando commented Sep 20, 2023

fatroom commented Sep 25, 2023

cberg-zalando Oct 2, 2023

Semernitskaya Oct 2, 2023

cberg-zalando Oct 2, 2023

cberg-zalando Oct 2, 2023

cberg-zalando commented Oct 2, 2023

fatroom commented Oct 5, 2023

add executor service param #1503

add executor service param #1503

Conversation

Semernitskaya commented Aug 27, 2023 • edited Loading

Description

Motivation and Context

Types of changes

Checklist:

fatroom commented Sep 11, 2023

Semernitskaya commented Sep 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Semernitskaya Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Semernitskaya Sep 18, 2023 • edited Loading

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 Sep 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Semernitskaya commented Sep 20, 2023 • edited Loading

cberg-zalando commented Sep 20, 2023

cberg-zalando commented Sep 20, 2023

fatroom commented Sep 25, 2023

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 Oct 2, 2023

fatroom commented Oct 5, 2023

Semernitskaya commented Aug 27, 2023 •

edited

Loading

Semernitskaya Sep 19, 2023 •

edited

Loading

Semernitskaya Sep 18, 2023 •

edited

Loading

cberg-zalando commented Sep 19, 2023 •

edited

Loading

Semernitskaya commented Sep 20, 2023 •

edited

Loading