Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Surface task scheduling results(success/failure) from SchedulerClient #326

Closed
cyuyang opened this issue Sep 23, 2022 · 4 comments · Fixed by #496
Closed

Surface task scheduling results(success/failure) from SchedulerClient #326

cyuyang opened this issue Sep 23, 2022 · 4 comments · Fixed by #496
Labels
enhancement pri1

Comments

@cyuyang
Copy link

cyuyang commented Sep 23, 2022

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.
@cyuyang cyuyang changed the title Surface task scheduling result(success/failure) form SchedulerClient Surface task scheduling results(success/failure) from SchedulerClient Sep 23, 2022
@kagkarlsson
Copy link
Owner

Agreed

@kagkarlsson
Copy link
Owner

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

@cyuyang
Copy link
Author

cyuyang commented Nov 22, 2022

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.

@kagkarlsson
Copy link
Owner

Picking this up. Yeah, on second thought, I will just add new methods and mark the other as deprecated.

@kagkarlsson kagkarlsson mentioned this issue May 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement pri1
Projects
None yet
Milestone
No milestone
2 participants
@kagkarlsson @cyuyang