feat(scheduler): all terminated tasks can have an output #2172
Conversation
| }); | ||
| expect(res.isOk()).toBe(false); | ||
| if (res.isErr()) { | ||
| expect(res.error.payload).toBe(res.error.payload); |
There was a problem hiding this comment.
this if is actually the only addition in the file. The rest is just grouping tests
| id: 1234, | ||
| provider_config_key: 'P', | ||
| environment_id: 5678 | ||
| describe('schedule', () => { |
There was a problem hiding this comment.
You can scroll to the end of this file where the only changes is. The rest is just grouping tests
| const t = await startTask(); | ||
| const updated = await tasks.transitionState({ taskId: t.id, newState: 'SUCCEEDED' }); | ||
| expect(updated.isErr()).toBe(true); | ||
| }); |
There was a problem hiding this comment.
no need anymore. It is now enforced at compile time.
| */ | ||
| public async fail({ taskId }: { taskId: string }): Promise<Result<Task>> { | ||
| const failed = await tasks.transitionState({ taskId, newState: 'FAILED' }); | ||
| public async fail({ taskId, error }: { taskId: string; error: JsonValue }): Promise<Result<Task>> { |
There was a problem hiding this comment.
error: JsonValue: error format is not enforced by the scheduler. The business logic (aka server and the processor) can define their own format. It is less prescriptive but allow for more flexibility.
TBonnin
force-pushed
the
tbonnin/tasks-output
branch
from
012c6b2
to
3410d93
Compare
| @@ -203,11 +204,11 @@ export class Scheduler { | |||
| * @example | |||
| * const cancelled = await scheduler.cancel({ taskId: '00000000-0000-0000-0000-000000000000' }); | |||
| */ | |||
| public async cancel(cancelBy: { taskId: string } | { scheduleId: string }): Promise<Result<Task>> { | |||
| public async cancel(cancelBy: { taskId: string; reason: string } | { scheduleId: string; reason: string }): Promise<Result<Task>> { | |||
There was a problem hiding this comment.
we could also make the second param a JsonValue and allow any kind of object. Let me know what you think.
| case 'CANCELLED': | ||
| return res.status(404).json({ error: { code: 'task_cancelled', message: `cancelled` } }); | ||
| return res.status(200).json({ state: task.value.state, output: task.value.output }); |
There was a problem hiding this comment.
FAILED and EXPIRED respond with a 200?
There was a problem hiding this comment.
yes, since this is the /output endpoint and there is potentially an output it is now a 200
| terminated: true | ||
| terminated: true, | ||
| output: db.raw(` | ||
| CASE |
There was a problem hiding this comment.
logic seems a bit verbose to be in a raw query. Unsure if there is a better way -- my only concern is maintenance
There was a problem hiding this comment.
I agree that it is verbose. Do you see a better way to achieve the same thing?
| */ | ||
| public async fail({ taskId }: { taskId: string }): Promise<Result<Task>> { | ||
| const failed = await tasks.transitionState({ taskId, newState: 'FAILED' }); | ||
| public async fail({ taskId, error }: { taskId: string; error: JsonValue }): Promise<Result<Task>> { |
There was a problem hiding this comment.
Not directly related to this PR but I feel if you destructured task it would be more readable in the fail function
public async fail({ taskId, error }: { taskId: string; error: JsonValue }): Promise<Result<Task>> {
const failed = await tasks.transitionState({ taskId, newState: 'FAILED', output: error });
if (failed.isOk()) {
const task = failed.value;
const { state, retryMax, retryCount, name, payload, groupKey, createdToStartedTimeoutSecs, startedToCompletedTimeoutSecs, heartbeatTimeoutSecs } =
task;
this.onCallbacks[state](task);
// Create a new task if the task is retryable
if (retryMax > retryCount) {
const schedulingProps: SchedulingProps = {
scheduling: 'immediate',
taskProps: {
name,
payload,
groupKey,
retryMax,
retryCount: retryCount + 1,
createdToStartedTimeoutSecs,
startedToCompletedTimeoutSecs,
heartbeatTimeoutSecs
}
};
const res = await this.schedule(schedulingProps);
if (res.isErr()) {
logger.error(`Error retrying task '${taskId}': ${stringifyError(res.error)}`);
}
}
}
return failed;
}
| @@ -113,20 +110,23 @@ describe('Task', () => { | |||
| await new Promise((resolve) => void setTimeout(resolve, timeout * 1100)); | |||
| const expired = (await tasks.expiresIfTimeout()).unwrap(); | |||
| expect(expired).toHaveLength(1); | |||
| expect(expired[0]?.output).toMatchObject({ reason: `Timeout 'createdToStartedTimeoutSecs' exceeded` }); | |||
There was a problem hiding this comment.
just a comment not a suggestion, usually I prefer storing error code (along with details if necessary) instead of full sentence it's more portable and easy to filter
TBonnin
force-pushed
the
tbonnin/tasks-output
branch
3 times, most recently
from
ad792ad
to
e55c3a5
Compare
I started with the idea to add an `output` for failed task but I realized all terminated task can have an output. Ex: reason for cancellation, etc... This commit ensures it is required to set an output when terminating a non successful task
TBonnin
force-pushed
the
tbonnin/tasks-output
branch
from
e55c3a5
to
21443b8
Compare
Describe your changes
I started with the idea to add an
outputfor failed task (the error that caused the failure) but I realized all terminated tasks can have an output. Ex: reason for cancellation, etc...This commit ensures it is required to set an output when terminating a non successful task
Checklist before requesting a review (skip if just adding/editing APIs & templates)