Repository: kagkarlsson/db-scheduler (java)

Issue:
Surface task scheduling results(success/failure) from SchedulerClient
Currently SchedulerClient#schedule() function hides the failure if a task is created as duplicates. 
```
@Override
public <T> void schedule(TaskInstance<T> taskInstance, Instant executionTime) {
    boolean success = taskRepository.createIfNotExists(SchedulableInstance.of(taskInstance, executionTime));
    if (success) {
        notifyListeners(ClientEvent.EventType.SCHEDULE, taskInstance, executionTime);
    }
}
```

It would be nice if the failure (or the boolean result) could be handed back to the user and let the user decide what action to take upon duplicates. Sometimes, duplicates are caused by programming bugs that are hard to detect if the failure is not surfaced.


Discussion:
Agreed
TBH, I feel the safe thing to do here is to throw an exception. This should be quite unusual. And introduce another method `scheduleIfNotExists`/`trySchedule` that will return boolean indicating if it succeeded or not
> TBH, I feel the safe thing to do here is to throw an exception. This should be quite unusual. And introduce another method `scheduleIfNotExists`/`trySchedule` that will return boolean indicating if it succeeded or not

Won't it be a breaking change for people that rely on it being silently ignored? Alternatively, we can keep the existing behavior in the method, and add new ones as you suggested.
Picking this up. Yeah, on second thought, I will just add new methods and mark the other as deprecated.

Patch:
diff --git a/db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/Scheduler.java b/db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/Scheduler.java
--- a/db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/Scheduler.java
+++ b/db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/Scheduler.java
@@ -252,6 +252,16 @@ public <T> void schedule(SchedulableInstance<T> schedulableInstance) {
     this.delegate.schedule(schedulableInstance);
   }
 
+  @Override
+  public <T> boolean scheduleIfNotExists(TaskInstance<T> taskInstance, Instant executionTime) {
+    return this.delegate.scheduleIfNotExists(taskInstance, executionTime);
+  }
+
+  @Override
+  public <T> boolean scheduleIfNotExists(SchedulableInstance<T> schedulableInstance) {
+    return this.delegate.scheduleIfNotExists(schedulableInstance);
+  }
+
   @Override
   public <T> void schedule(TaskInstance<T> taskInstance, Instant executionTime) {
     this.delegate.schedule(taskInstance, executionTime);
diff --git a/db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/SchedulerClient.java b/db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/SchedulerClient.java
--- a/db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/SchedulerClient.java
+++ b/db-scheduler/src/main/java/com/github/kagkarlsson/scheduler/SchedulerClient.java
@@ -40,17 +40,43 @@
 public interface SchedulerClient {
 
   /**
-   * Schedule a new execution.
+   * Schedule a new execution if task instance does not already exists.
    *
    * @param taskInstance Task-instance, optionally with data
    * @param executionTime Instant it should run
    * @see java.time.Instant
    * @see com.github.kagkarlsson.scheduler.task.TaskInstance
+   * @deprecated use {@link #scheduleIfNotExists(TaskInstance, Instant)} instead.
    */
+  @Deprecated
   <T> void schedule(TaskInstance<T> taskInstance, Instant executionTime);
 
+  /**
+   * @deprecated use {@link #scheduleIfNotExists(SchedulableInstance)} instead.
+   */
+  @Deprecated
   <T> void schedule(SchedulableInstance<T> schedulableInstance);
 
+  /**
+   * Schedule a new execution if task instance does not already exists.
+   *
+   * @param taskInstance Task-instance, optionally with data
+   * @param executionTime Instant it should run
+   * @see java.time.Instant
+   * @see com.github.kagkarlsson.scheduler.task.TaskInstance
+   * @return true if scheduled successfully
+   */
+  <T> boolean scheduleIfNotExists(TaskInstance<T> taskInstance, Instant executionTime);
+
+  /**
+   * Schedule a new execution if task instance does not already exists.
+   *
+   * @param schedulableInstance Task-instance and time it should run
+   * @see com.github.kagkarlsson.scheduler.task.SchedulableInstance
+   * @return true if scheduled successfully
+   */
+  <T> boolean scheduleIfNotExists(SchedulableInstance<T> schedulableInstance);
+
   /**
    * Update an existing execution to a new execution-time. If the execution does not exist or if it
    * is currently running, an exception is thrown.
@@ -256,11 +282,25 @@ class StandardSchedulerClient implements SchedulerClient {
 
     @Override
     public <T> void schedule(TaskInstance<T> taskInstance, Instant executionTime) {
+      // ignore result even if failed to schedule due to duplicates for backwards-compatibility
+      scheduleIfNotExists(taskInstance, executionTime);
+    }
+
+    @Override
+    public <T> boolean scheduleIfNotExists(TaskInstance<T> taskInstance, Instant executionTime) {
       boolean success =
           taskRepository.createIfNotExists(SchedulableInstance.of(taskInstance, executionTime));
       if (success) {
         notifyListeners(ClientEvent.EventType.SCHEDULE, taskInstance, executionTime);
       }
+      return success;
+    }
+
+    @Override
+    public <T> boolean scheduleIfNotExists(SchedulableInstance<T> schedulableInstance) {
+      return scheduleIfNotExists(
+          schedulableInstance.getTaskInstance(),
+          schedulableInstance.getNextExecutionTime(clock.now()));
     }
 
     @Override


Test patch:
diff --git a/db-scheduler/src/test/java/com/github/kagkarlsson/scheduler/SchedulerClientTest.java b/db-scheduler/src/test/java/com/github/kagkarlsson/scheduler/SchedulerClientTest.java
--- a/db-scheduler/src/test/java/com/github/kagkarlsson/scheduler/SchedulerClientTest.java
+++ b/db-scheduler/src/test/java/com/github/kagkarlsson/scheduler/SchedulerClientTest.java
@@ -5,6 +5,7 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.*;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import co.unruly.matchers.OptionalMatchers;
@@ -79,10 +80,20 @@ public void setUp() {
   @Test
   public void client_should_be_able_to_schedule_executions() {
     SchedulerClient client = create(DB.getDataSource()).build();
+
+    // test deprecated method
     client.schedule(oneTimeTaskA.instance("1"), settableClock.now());
+    assertFalse(client.scheduleIfNotExists(oneTimeTaskA.instance("1"), settableClock.now()));
 
     scheduler.runAnyDueExecutions();
     assertThat(onetimeTaskHandlerA.timesExecuted.get(), CoreMatchers.is(1));
+
+    // test new method
+    client.scheduleIfNotExists(oneTimeTaskA.instance("1"), settableClock.now());
+    assertFalse(client.scheduleIfNotExists(oneTimeTaskA.instance("1"), settableClock.now()));
+
+    scheduler.runAnyDueExecutions();
+    assertThat(onetimeTaskHandlerA.timesExecuted.get(), CoreMatchers.is(2));
   }
 
   @Test