Allow setting onFailure for RecurringTaskWithPersistentSchedule

[olayinkasf]

No description provided.

[kagkarlsson]

Makes sense. What is your particular need?

[olayinkasf]

@kagkarlsson Without the ability to set a failure handler, I had to do this which is kinda hacky

  public record ScheduleData(ZoneId zoneId, LocalTime time, String id)
      implements ScheduleAndData, Schedule {

    @Override
    public Schedule getSchedule() {
      return this;
    }

    @Override
    public String getData() {
      return id;
    }

    @Override
    public Instant getNextExecutionTime(ExecutionComplete executionComplete) {
      if (executionComplete.getResult() == Result.FAILED)
        return executionComplete.getTimeDone().plus(RETRY_DELAY);
      return Schedules.daily(zoneId, time).getNextExecutionTime(executionComplete);
    }

    @Override
    public boolean isDeterministic() {
      return true;
    }
  }

It would be nice to not have to override the getNextExecutionTime method, keeping the code clean and consistent.

EDIT

It's only a tiny change. There are a few other tiny improvements observed and I'm not certain what the recommendations are. Batch them in a single PR or separate in multiple PRs?

[olayinkasf]

The changes proposed:

• Allow setting onFailure when building RecurringTaskWithPersistentSchedule.
• Allow supplying just onFailure to the constructors. I have about 14 task classes all inheriting from different tasks and having different failure handlers. When inheriting from the tasks, I had to supply both the onFailure and dead execution handler which is inconvenient given that, all 14 task classes revive dead executions. Having the opportunity to supply just the onFailure in the constructor reduces the additional noisy code.
• Allow setting the TaskResolver via the builder. It is currently not possible to handle the case where a task depends on the scheduler. Having the TaskResolver independent of the scheduler allows us to add and remove tasks any time we want especially after building the scheduler.
• Allow setting the clock via the builder which allows the scheduler to use the same clock as the rest of the app. Creating the waiter which depends on the clock and the polling interval is then deferred until the build method is called.

[kagkarlsson]

Will have a look at this on Friday

[kagkarlsson]

It's only a tiny change. There are a few other tiny improvements observed and I'm not certain what the recommendations are. Batch them in a single PR or separate in multiple PRs?

I think split out task-resolver and clock stuff into a separate PR, then this PR will be fine. I might merge it and remove those bits, to be able to merge the FailureHandler change today.

[kagkarlsson]

I will merge this PR, but remove the clock, task-resolver change. I think those changes will block the merge otherwise, and was hoping to release a new version today

[kagkarlsson]

I am uncertain what the use-case for this is. If you do not use the default or a variant of it not much will work

[kagkarlsson]

I see you have commented a bit on it. We can take the discussion in another PR

[kagkarlsson]

What would be the use-case for this?

[kagkarlsson]

I see you have commented a bit on it. We can take the discussion in another PR

[kagkarlsson]

I think we need to discuss this:

Allow setting the TaskResolver via the builder. It is currently not possible to handle the case where a task depends on the scheduler. Having the TaskResolver independent of the scheduler allows us to add and remove tasks any time we want especially after building the scheduler.

To avoid circular dependency, you can get a SchedulerClient from executionContext.getSchedulerClient() in the ExecutionHandler