feat: Doomslug endorsement delay starts from the last endorsement, no… #3164
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…t the head update, and test fixes Making doomslug wait for 600ms from the last time the node sent an endorsement, not from the head update. Under load this will remove unnecessary idleness. This change breaks catchup tests that do not use doomslug, because they rely on blocks being produced without skips and forks, and without doomslug and the wait not starting from the head update the timing is less predictable. I address it by making the tests actually use doomslug (and upping the block prod times where necessary). Since they rely on having no skips and forks, there's no reason not to use doomslug. For the only test that had two separate versions with and without doomslug, I removed the version without. Separately and unrelatedly to the above, removed backtrace from the formatted client errors. On my (very beefy) machine fetching the backtrace takes 800ms, and on nayduck runners it was more than 2s. Since we format errors in many places (including responces to RPC, though that will change), such delays are not acceptable. This fixes state_sync tests. Fixes: #3139, #3129 Test plan: ---------- For #3139: http://nayduck.eastus.cloudapp.azure.com:3000/#/run/88 For #3129: (still running, will update the description) Catchup tests are split accross many runs, but I ensured they are not flaky after this change.
|
|
Codecov Report
@@ Coverage Diff @@
## master #3164 +/- ##
==========================================
+ Coverage 87.42% 87.46% +0.04%
==========================================
Files 212 212
Lines 41556 41552 -4
==========================================
+ Hits 36330 36345 +15
+ Misses 5226 5207 -19
Continue to review full report at Codecov.
|
bowenwang1996
approved these changes
Aug 14, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…t the head update, and test fixes
Making doomslug wait for 600ms from the last time the node sent an endorsement, not from the head update. Under load this will remove unnecessary idleness.
This change breaks catchup tests that do not use doomslug, because they rely on blocks being produced without skips and forks, and without doomslug and the wait not starting from the head update the timing is less predictable.
I address it by making the tests actually use doomslug (and upping the block prod times where necessary). Since they rely on having no skips and forks, there's no reason not to use doomslug.
For the only test that had two separate versions with and without doomslug, I removed the version without.
Separately and unrelatedly to the above, removed backtrace from the formatted client errors. On my (very beefy) machine fetching the backtrace takes 800ms, and on nayduck runners it was more than 2s. Since we format errors in many places (including responces to RPC, though that will change), such delays are not acceptable.
This fixes state_sync tests.
Fixes: #3139, #3129
Test plan:
For #3139: http://nayduck.eastus.cloudapp.azure.com:3000/#/run/88
For #3129: http://nayduck.eastus.cloudapp.azure.com:3000/#/run/90
Catchup tests are split accross many runs, but I ensured they are not flaky after this change.