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

go/worker/compute/executor: Minor bug fixes #5404

Merged
merged 5 commits into from
Oct 18, 2023

Conversation

peternose
Copy link
Contributor

@peternose peternose commented Oct 13, 2023

No description provided.

@peternose peternose changed the title Peternose/bugfix/fix bakcup workers go/worker/compute/executor: Minor bug fixes Oct 13, 2023
@peternose peternose force-pushed the peternose/bugfix/fix-bakcup-workers branch from 782d5f9 to 9c099a1 Compare October 16, 2023 07:54
The timestamp of the last block lags behind and depends on the consensus
layer's speed. As a result, it is not suitable for scheduling backup
proposers.
@peternose peternose force-pushed the peternose/bugfix/fix-bakcup-workers branch from 9c099a1 to 60aa2c7 Compare October 16, 2023 07:54
Transition to a new state once all transactions are fetched. This will
immediately trigger batch processing.
@peternose peternose force-pushed the peternose/bugfix/fix-bakcup-workers branch from 60aa2c7 to ccb26b5 Compare October 16, 2023 09:36
@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

Merging #5404 (ccb26b5) into master (c8f3c56) will decrease coverage by 0.42%.
Report is 1 commits behind head on master.
The diff coverage is 85.91%.

❗ Current head ccb26b5 differs from pull request most recent head 26ffcfb. Consider uploading reports for the commit 26ffcfb to get more accurate results

@@            Coverage Diff             @@
##           master    #5404      +/-   ##
==========================================
- Coverage   66.55%   66.14%   -0.42%     
==========================================
  Files         534      533       -1     
  Lines       56325    56326       +1     
==========================================
- Hits        37489    37257     -232     
- Misses      14414    14643     +229     
- Partials     4422     4426       +4     
Files Coverage Δ
...o/consensus/cometbft/apps/roothash/finalization.go 74.61% <100.00%> (-0.90%) ⬇️
go/worker/client/committee/node.go 66.32% <ø> (-0.18%) ⬇️
go/worker/common/committee/node.go 60.13% <ø> (-1.93%) ⬇️
...o/worker/compute/executor/committee/discrepancy.go 95.45% <100.00%> (-0.91%) ⬇️
go/worker/compute/executor/committee/hooks.go 90.90% <ø> (-5.52%) ⬇️
go/worker/compute/executor/committee/metrics.go 66.66% <ø> (ø)
go/worker/storage/committee/node.go 73.07% <ø> (-0.85%) ⬇️
go/roothash/api/commitment/pool.go 92.82% <75.00%> (-0.10%) ⬇️
.../worker/compute/executor/committee/transactions.go 67.60% <71.42%> (-10.83%) ⬇️
go/worker/compute/executor/committee/node.go 72.62% <89.52%> (-1.76%) ⬇️

... and 63 files with indirect coverage changes

@peternose peternose marked this pull request as ready for review October 16, 2023 16:17
Calculating the pool rank from roothash events does not prevent backup
schedulers from starting when higher-ranked schedulers have already
committed to their proposals, especially when the proposer timeout
is shorter than the time required to generate a consensus block.
Therefore, it is more suitable to estimate the rank from observed
executor commitments.
@peternose peternose force-pushed the peternose/bugfix/fix-bakcup-workers branch from ccb26b5 to 23e2205 Compare October 17, 2023 08:12
@kostko
Copy link
Member

kostko commented Oct 17, 2023

Also backport to stable/23.0.x.

In unit tests, CometBFT typically generates consensus blocks
at a rate of 10 per second. However, if consensus timeout occurs
in steps RoundStepPropose or RoundStepPrecommitWait, this rate
can significantly decrease to just 1 block per second or even
worse. If test requires waiting for a few consensus blocks,
these delays can accumulate and potentially lead to failures.
@peternose peternose enabled auto-merge October 18, 2023 10:06
@peternose peternose merged commit f63c447 into master Oct 18, 2023
1 check passed
@peternose peternose deleted the peternose/bugfix/fix-bakcup-workers branch October 18, 2023 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants