Skip to content

Add exponential backoff for task scheduling with task-level retries#11940

Merged
losipiuk merged 2 commits intotrinodb:masterfrom
losipiuk:lo/task-retries-backoff
Apr 15, 2022
Merged

Add exponential backoff for task scheduling with task-level retries#11940
losipiuk merged 2 commits intotrinodb:masterfrom
losipiuk:lo/task-retries-backoff

Conversation

@losipiuk
Copy link
Member

@losipiuk losipiuk commented Apr 13, 2022

Add exponential backoff for task scheduling with task-level retries

Description

Is this change a fix, improvement, new feature, refactoring, or other?

enhancement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

core

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

@losipiuk losipiuk requested a review from arhimondr April 13, 2022 13:49
@cla-bot cla-bot bot added the cla-signed label Apr 13, 2022
Copy link
Contributor

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

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

LGTM % session properties and testing

Copy link
Contributor

Choose a reason for hiding this comment

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

There's RETRY_INITIAL_DELAY, RETRY_MAX_DELAY session properties. What do you think about reusing those? (maybe also re-adjust the defaults, starting with 1m and capping at 5m)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I started adding new properties yesterday - but I think reusing those is fine. It could be more elastic to have separate ones for case when queries with query-level and task-level retries are run in the same cluster - but it is not a production supported case right now.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's no session property for exponential growth base though. Do we want to have it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Will add

@losipiuk losipiuk force-pushed the lo/task-retries-backoff branch 2 times, most recently from 624ad34 to cacdf60 Compare April 14, 2022 14:37
@losipiuk
Copy link
Member Author

Added tests

@losipiuk losipiuk marked this pull request as ready for review April 14, 2022 14:37
@losipiuk losipiuk requested a review from linzebing April 14, 2022 14:37
@losipiuk losipiuk force-pushed the lo/task-retries-backoff branch from cacdf60 to b8c794b Compare April 14, 2022 21:38
@losipiuk
Copy link
Member Author

AC and added one test

@losipiuk losipiuk requested review from arhimondr and linzebing April 14, 2022 21:39
Copy link
Contributor

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

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

LGTM % nits

@losipiuk losipiuk force-pushed the lo/task-retries-backoff branch from b8c794b to 66b03cb Compare April 15, 2022 09:04
@losipiuk
Copy link
Member Author

added docs

@losipiuk losipiuk force-pushed the lo/task-retries-backoff branch from 66b03cb to 3216b31 Compare April 15, 2022 10:05
@github-actions github-actions bot added the docs label Apr 15, 2022
@losipiuk
Copy link
Member Author

CI: #11784

@losipiuk losipiuk merged commit 7525907 into trinodb:master Apr 15, 2022
@losipiuk
Copy link
Member Author

Merged. Thanks for review @linzebing @arhimondr

@github-actions github-actions bot added this to the 378 milestone Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants