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.

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

Modify task manager getRetryDelay to be more custom #42064

Merged
merged 14 commits into from
Jul 29, 2019

Conversation

mikecote
Copy link
Contributor

@mikecote mikecote commented Jul 26, 2019

In this PR, I'm changing getRetryDelay to 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 getRetry and can return true, false or a Date. When returning true, this tells the task manager to retry the task using the default delay logic. When returning false, 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.

@mikecote mikecote requested review from pmuellr and a team July 26, 2019 13:33
@mikecote mikecote self-assigned this Jul 26, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-stack-services

@elasticmachine

This comment has been minimized.

@mikecote
Copy link
Contributor Author

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).

@elasticmachine

This comment has been minimized.

Copy link
Contributor

@bmcconaghy bmcconaghy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, just a suggestion for the tests.

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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicating the default retry logic in the test seems bad. Can you call the actual implementation to get this value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confusion, this variable is to contain the expected result returned from the implementation. I went ahead and renamed it to expectedRunAt.

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - left one comment about use of a constructed Boom error - could use at least a comment, slightly confusing why it's there

? intervalFromNow(this.definition.timeout)!
: this.getRetryDelay({
attempts,
error: Boom.clientTimeout(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ... :-)

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@mikecote mikecote merged commit 4ac8a19 into elastic:master Jul 29, 2019
mikecote added a commit to mikecote/kibana that referenced this pull request Jul 29, 2019
* 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
mikecote added a commit that referenced this pull request Jul 29, 2019
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants