Modify task manager getRetryDelay to be more custom #42064
Conversation
mikecote
added
v8.0.0
release_note:skip
|
Pinging @elastic/kibana-stack-services |
This comment has been minimized.
This comment has been minimized.
|
It was discussed with @bmcconaghy and @pmuellr that tasks with intervals should not have retry logic, it should get scheduled to its next interval like if it was successful. #39349 (comment). |
This comment has been minimized.
This comment has been minimized.
| const instance = store.update.args[0][0]; | ||
|
|
||
| expect(instance.runAt.getTime()).toEqual(secondsFromNow(retryDelay).getTime()); | ||
| const nextDefaultRetryAt = new Date(Date.now() + initialAttempts * 5 * 60 * 1000); |
There was a problem hiding this comment.
Duplicating the default retry logic in the test seems bad. Can you call the actual implementation to get this value?
There was a problem hiding this comment.
Sorry for the confusion, this variable is to contain the expected result returned from the implementation. I went ahead and renamed it to expectedRunAt.
| ? intervalFromNow(this.definition.timeout)! | ||
| : this.getRetryDelay({ | ||
| attempts, | ||
| error: Boom.clientTimeout(), |
There was a problem hiding this comment.
slightly weird to provide a bogus error to force the path in this.getRetryDelay(). Would it make sense to allow null instead? Maybe not, more type madness. If not, I think it's probably comment-worthy why the error is passed in - I suppose it could be in the "message" of the error ... :-)
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
💚 Build Succeeded |
* Modify task manager getRetryDelay to be more custom * Ensure intervals don't die * Update comment * Update comment pt2 * Fix type check * Intervals to not have retry logic * Make test easier to read * Modify docs * Rename confusing variable * Apply PR feedback * Change error to task timeout error * Re-add comment * Fix type check
* Modify task manager getRetryDelay to be more custom * Ensure intervals don't die * Update comment * Update comment pt2 * Fix type check * Intervals to not have retry logic * Make test easier to read * Modify docs * Rename confusing variable * Apply PR feedback * Change error to task timeout error * Re-add comment * Fix type check
In this PR, I'm changing
getRetryDelayto be more custom than just returning a number in seconds. That number used to tell task manager to wait x seconds before trying the task again.The function is renamed to
getRetryand can returntrue,falseor a Date. When returningtrue, this tells the task manager to retry the task using the default delay logic. When returningfalse, this tells the task manager to stop retrying the task. When returning a date, this suggests to the task manager when to retry the task. This function isn't used for interval type tasks, those retry at the next interval.Interval type tasks will no longer have any retry logic, they will be retried at their next interval.