-
Notifications
You must be signed in to change notification settings - Fork 427
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.
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FAILED
and EXPIRED
respond with a 200?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non blocking feedback and q's
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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
e55c3a5
to
21443b8
Compare
Describe your changes
I started with the idea to add an
output
for 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)