-
Notifications
You must be signed in to change notification settings - Fork 136
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
Reduce interval to re-trigger app validation workflow to 100-1000 milliseconds #3884
Reduce interval to re-trigger app validation workflow to 100-1000 milliseconds #3884
Conversation
// Trigger app validation workflow again in 10 seconds. | ||
WorkComplete::Incomplete(Some(Duration::from_secs(10))) | ||
// Trigger app validation workflow again in 20-2000 milliseconds. | ||
let interval = max(101 - validations_dependencies.missing_hashes.len(), 1) * 20; |
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.
There are lots of formulas for this interval. I don't even have enough data to decide if this adaptation to number of missing hashes makes sense or if a fixed interval of 1 second is equally suitable.
@@ -27,7 +27,7 @@ impl Default for ValidationDependencies { | |||
} | |||
|
|||
impl ValidationDependencies { | |||
const FETCH_TIMEOUT: Duration = Duration::from_secs(60); | |||
const FETCH_TIMEOUT: Duration = Duration::from_secs(30); |
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.
Why?
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.
My reasoning is that if there are a few hashes that cannot be fetched, app validation will retrigger every couple of 100 ms. It would continue for 60 seconds, so I thought 30 seconds is a compromise between a frequent re-trigger and enough time to fetch hashes from a slow node.
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 seems inconsistent given that we allow 60 sec for fetching data in other contexts. It seems like if we are willing to reduce the timeout here, we should reduce it everywhere, because similar reasoning applies in other situations, especially because it's been asserted that running validation is not very expensive, so a constant factor of 2 in worst-case time difference should not have that much effect compared to the actual effect of waiting 60 seconds to determine the fetchability of some data. i.e. if it's undesirable to wait 60 seconds here, it seems equally undesirable everywhere else.
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.
Fine, agreed.
…r/reduce-interval-before-re-triggering-app-validation
…ring-app-validation
…ring-app-validation
…ring-app-validation
…ring-app-validation
…00 ms (#3884) * refactor(app-validation-wf): reduce interval to re-trigger to 20-2000 ms * refactor: update re-trigger formula & lower fetch timeout * revert fetch timeout * fix interval calculation * Update CHANGELOG.md
…to 100-1000 milliseconds (#3999) * refactor(app-validation-wf): reduce interval to re-trigger to 99-1000 ms (#3884) * refactor(app-validation-wf): reduce interval to re-trigger to 20-2000 ms * refactor: update re-trigger formula & lower fetch timeout * revert fetch timeout * fix interval calculation * Update CHANGELOG.md Co-authored-by: Jost Schulte <jost.schulte@protonmail.com>
Summary
I think the title is a summary of this small PR.
TODO: