From 1432e9da2cbcc5b4e92b69e01d6c54a45b0a32bd Mon Sep 17 00:00:00 2001
From: Benedikt Beil <beil@crxmarkets.com>
Date: Tue, 12 Nov 2024 17:12:59 +0100
Subject: [PATCH] Improve flexibility of the MaxRetriesFailureHandler

---
 README.md                                     |  3 ++
 .../scheduler/task/FailureHandler.java        |  8 ++++--
 .../scheduler/task/FailureHandlerTest.java    | 28 +++++++++++++++++++
 3 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/README.md b/README.md
index 74af727b..941c4814 100644
--- a/README.md
+++ b/README.md
@@ -579,6 +579,9 @@ There are a number of users that are using db-scheduler for high throughput use-
 
 See [releases](https://github.com/kagkarlsson/db-scheduler/releases) for release-notes.
 
+**Upgrading to 15.x**
+* The behavior of tasks with a defined `maxRetriesExceededHandler` for the `MaxRetriesFailureHandler` has been updated for greater flexibility. Tasks will no longer be automatically removed. To maintain the previous behavior of task removal, please invoke `executionOperations.stop()` within your `maxRetriesExceededHandler`. If you have not defined a `maxRetriesExceededHandler`, the behavior remains unchanged, and no further action is required.
+
 **Upgrading to 8.x**
 * Custom Schedules must implement a method `boolean isDeterministic()` to indicate whether they will always produce the same instants or not.
 
diff --git a/db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/task/FailureHandler.java b/db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/task/FailureHandler.java
index 2f191c3b..848e3ac7 100644
--- a/db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/task/FailureHandler.java
+++ b/db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/task/FailureHandler.java
@@ -70,7 +70,10 @@ class MaxRetriesFailureHandler<T> implements FailureHandler<T> {
     private final BiConsumer<ExecutionComplete, ExecutionOperations<T>> maxRetriesExceededHandler;
 
     public MaxRetriesFailureHandler(int maxRetries, FailureHandler<T> failureHandler) {
-      this(maxRetries, failureHandler, (executionComplete, executionOperations) -> {});
+      this(
+          maxRetries,
+          failureHandler,
+          (executionComplete, executionOperations) -> executionOperations.stop());
     }
 
     public MaxRetriesFailureHandler(
@@ -90,10 +93,9 @@ public void onFailure(
       int totalNumberOfFailures = consecutiveFailures + 1;
       if (totalNumberOfFailures > maxRetries) {
         LOG.error(
-            "Execution has failed {} times for task instance {}. Cancelling execution.",
+            "Execution has failed {} times for task instance {}. Invoking maxRetriesExceededHandler",
             totalNumberOfFailures,
             executionComplete.getExecution().taskInstance);
-        executionOperations.stop();
         maxRetriesExceededHandler.accept(executionComplete, executionOperations);
       } else {
         this.failureHandler.onFailure(executionComplete, executionOperations);
diff --git a/db-scheduler/src/test/java/com/github/kagkarlsson/scheduler/task/FailureHandlerTest.java b/db-scheduler/src/test/java/com/github/kagkarlsson/scheduler/task/FailureHandlerTest.java
index 1185d69b..02e4556f 100644
--- a/db-scheduler/src/test/java/com/github/kagkarlsson/scheduler/task/FailureHandlerTest.java
+++ b/db-scheduler/src/test/java/com/github/kagkarlsson/scheduler/task/FailureHandlerTest.java
@@ -56,9 +56,37 @@ void should_run_handler_when_max_retries_exceeded() {
 
       assertThat(failureHandlerCalled.get(), is(false));
       assertThat(maxRetriesExceededHandlerCalled.get(), is(true));
+    }
+
+    @Test
+    void should_remove_task_when_maxRetriesExceededHandler_is_undefined_and_max_retries_exceeded() {
+      MaxRetriesFailureHandler<String> maxRetriesFailureHandler =
+          new MaxRetriesFailureHandler<>(3, failureHandler);
+
+      Execution execution = getExecutionWithFails(3);
+      when(executionComplete.getExecution()).thenReturn(execution);
+
+      maxRetriesFailureHandler.onFailure(executionComplete, executionOperations);
+
+      assertThat(failureHandlerCalled.get(), is(false));
       verify(executionOperations).stop();
     }
 
+    @Test
+    void should_keep_task_when_maxRetriesExceededHandler_is_defined_and_max_retries_exceeded() {
+      MaxRetriesFailureHandler<String> maxRetriesFailureHandler =
+          new MaxRetriesFailureHandler<>(3, failureHandler, maxRetriesExceededHandler);
+
+      Execution execution = getExecutionWithFails(3);
+      when(executionComplete.getExecution()).thenReturn(execution);
+
+      maxRetriesFailureHandler.onFailure(executionComplete, executionOperations);
+
+      assertThat(failureHandlerCalled.get(), is(false));
+      assertThat(maxRetriesExceededHandlerCalled.get(), is(true));
+      verifyNoInteractions(executionOperations);
+    }
+
     @Test
     void should_do_not_run_max_retries_handler_when_retries_are_not_exceeded() {
       MaxRetriesFailureHandler<String> maxRetriesFailureHandler =