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

[Block Executor] Follow up fix-ups #12468

Merged
merged 1 commit into from
Mar 12, 2024
Merged

[Block Executor] Follow up fix-ups #12468

merged 1 commit into from
Mar 12, 2024

Conversation

gelash
Copy link
Contributor

@gelash gelash commented Mar 11, 2024

A follow-up for recent changes to clean up some things in the scheduler and executor.

Copy link

trunk-io bot commented Mar 11, 2024

⏱️ 16h 50m total CI duration on this PR
Job Cumulative Duration Recent Runs
rust-unit-coverage 4h 23m 🟩
rust-smoke-coverage 3h 40m 🟩
windows-build 1h 58m 🟩🟩🟩🟩🟩
execution-performance / single-node-performance 1h 30m 🟩🟩🟥🟩
rust-smoke-tests 1h 12m 🟩🟩
rust-unit-tests 1h 7m 🟩🟩
rust-images / rust-all 35m 🟩🟩
forge-compat-test / forge 32m 🟩🟩
forge-e2e-test / forge 31m 🟩🟩
rust-lints 19m 🟩🟩
run-tests-main-branch 13m 🟩🟩🟩
check 12m 🟩🟩🟩
cli-e2e-tests / run-cli-tests 12m 🟩🟩
check-dynamic-deps 9m 🟩🟩🟩🟩🟩
general-lints 6m 🟩🟩🟩
indexer-grpc-e2e-tests / test-indexer-grpc-docker-compose 5m 🟩🟩
semgrep/ci 2m 🟩🟩🟩🟩🟩
node-api-compatibility-tests / node-api-compatibility-tests 2m 🟩🟩
file_change_determinator 40s 🟩🟩🟩🟩
execution-performance / file_change_determinator 37s 🟩🟩🟩🟩
file_change_determinator 34s 🟩🟩🟩
file_change_determinator 32s 🟩🟩🟩
permission-check 16s 🟩🟩🟩🟩🟩
upload-to-codecov 16s 🟩
permission-check 15s 🟩🟩🟩🟩🟩
permission-check 14s 🟩🟩🟩
permission-check 14s 🟩🟩🟩🟩🟩
permission-check 13s 🟩🟩🟩🟩🟩
determine-docker-build-metadata 6s 🟩🟩🟩

🚨 1 job on the last run was significantly faster/slower than expected

Job Duration vs 7d avg Delta
windows-build 27m 20m +30%

settingsfeedbackdocs ⋅ learn more about trunk.io

@gelash gelash added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Mar 11, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@@ -81,13 +81,13 @@ pub enum ExecutionTaskType {

/// A holder for potential task returned from the Scheduler. ExecutionTask and ValidationTask
/// each contain a version of transaction that must be executed or validated, respectively.
/// NoTask holds no task (similar None if we wrapped tasks in Option), and Done implies that
/// Retry holds no task (similar None if we wrapped tasks in Option), and Done implies that
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: these explanations can go together with the enum variants, then easier to keep comments up to date?

/// Holds no task (similar None if we wrapped tasks in Option).
Retry,
/// Implies that there are no more tasks and the scheduler is done.
Done,

aptos-move/block-executor/src/scheduler.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.9%. Comparing base (e492ecd) to head (e2b3730).
Report is 2 commits behind head on main.

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

Additional details and impacted files
@@            Coverage Diff            @@
##             main   #12468     +/-   ##
=========================================
- Coverage    70.3%    69.9%   -0.4%     
=========================================
  Files        2269     2269             
  Lines      428002   428006      +4     
=========================================
- Hits       300940   299416   -1524     
- Misses     127062   128590   +1528     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gelash gelash force-pushed the gelash/blokexfixup branch from e2b3730 to 141dc2e Compare March 12, 2024 16:47
@gelash gelash force-pushed the gelash/blokexfixup branch from 141dc2e to 7466818 Compare March 12, 2024 16:52
@gelash gelash enabled auto-merge (squash) March 12, 2024 16:52

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite compat success on aptos-node-v1.9.5 ==> 7466818f6adf375f7db521857cf3821fb8ceb201

Compatibility test results for aptos-node-v1.9.5 ==> 7466818f6adf375f7db521857cf3821fb8ceb201 (PR)
1. Check liveness of validators at old version: aptos-node-v1.9.5
compatibility::simple-validator-upgrade::liveness-check : committed: 6604 txn/s, latency: 5020 ms, (p50: 4800 ms, p90: 8700 ms, p99: 10200 ms), latency samples: 231160
2. Upgrading first Validator to new version: 7466818f6adf375f7db521857cf3821fb8ceb201
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 701 txn/s, latency: 34529 ms, (p50: 36100 ms, p90: 54100 ms, p99: 56400 ms), latency samples: 57500
3. Upgrading rest of first batch to new version: 7466818f6adf375f7db521857cf3821fb8ceb201
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 247 txn/s, submitted: 577 txn/s, expired: 329 txn/s, latency: 33167 ms, (p50: 46100 ms, p90: 59100 ms, p99: 76000 ms), latency samples: 20826
4. upgrading second batch to new version: 7466818f6adf375f7db521857cf3821fb8ceb201
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 2473 txn/s, latency: 11788 ms, (p50: 12000 ms, p90: 16300 ms, p99: 17500 ms), latency samples: 116260
5. check swarm health
Compatibility test for aptos-node-v1.9.5 ==> 7466818f6adf375f7db521857cf3821fb8ceb201 passed
Test Ok

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 7466818f6adf375f7db521857cf3821fb8ceb201

two traffics test: inner traffic : committed: 7976 txn/s, latency: 4925 ms, (p50: 4800 ms, p90: 5700 ms, p99: 9600 ms), latency samples: 3438040
two traffics test : committed: 100 txn/s, latency: 1881 ms, (p50: 1800 ms, p90: 2100 ms, p99: 4900 ms), latency samples: 1740
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.224, avg: 0.203", "QsPosToProposal: max: 0.259, avg: 0.240", "ConsensusProposalToOrdered: max: 0.441, avg: 0.411", "ConsensusOrderedToCommit: max: 0.306, avg: 0.294", "ConsensusProposalToCommit: max: 0.718, avg: 0.705"]
Max round gap was 1 [limit 4] at version 1586676. Max no progress secs was 4.27625 [limit 15] at version 1586676.
Test Ok

@gelash gelash merged commit 0d6829d into main Mar 12, 2024
54 of 55 checks passed
@gelash gelash deleted the gelash/blokexfixup branch March 12, 2024 17:29
igor-aptos pushed a commit that referenced this pull request Mar 19, 2024
igor-aptos added a commit that referenced this pull request Mar 20, 2024
* [Block Executor] Halt executing, utilize PanicError, clean-up (#12230)

* [Block Executor] Follow up fix-ups (#12468)

---------
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants