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.

Sign up for GitHub

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

Jump to bottom

Deprecate DelayedQcMsg and simplify the code #14640

Merged
merged 1 commit into from
Sep 18, 2024
Merged

Deprecate DelayedQcMsg and simplify the code #14640

merged 1 commit into from
Sep 18, 2024

Conversation

vusirikala
Copy link
Contributor

@vusirikala vusirikala commented Sep 15, 2024 •
edited
Loading

Description

Deprecating DelayedQCMsg. We implemented this over a year ago to experiment. But we are not using it in production. Simplifying the code by removing this.

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Other (specify)

How Has This Been Tested?

Key Areas to Review

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

@trunk-io Trunk.io
Copy link

trunk-io bot commented Sep 15, 2024 •
edited
Loading

⏱️ 3h 55m total CI duration on this PR
Job Cumulative Duration Recent Runs
execution-performance / single-node-performance 47m 🟩 🟩 
forge-e2e-test / forge 42m 🟩 🟥 🟩 
forge-compat-test / forge 36m 🟩 🟩 
forge-framework-upgrade-test / forge 16m 🟩 
rust-images / rust-all 13m 🟩 
execution-performance / test-target-determinator 13m 🟩 🟩 
test-target-determinator 12m 🟩 🟩 
rust-move-tests 10m 🟩 
rust-move-tests 9m 🟩 
check 8m 🟩 🟩 
rust-doc-tests 5m 🟩 
rust-doc-tests 5m 🟩 
general-lints 4m ⬜ 🟩 🟩 
rust-cargo-deny 4m ⬜ 🟩 🟩 
indexer-grpc-e2e-tests / test-indexer-grpc-docker-compose 4m 🟩 🟩 
check-dynamic-deps 3m 🟩 🟩 🟩 
semgrep/ci 1m 🟩 🟩 🟩 
file_change_determinator 33s 🟩 🟩 🟩 
file_change_determinator 30s 🟩 🟩 🟩 
file_change_determinator 24s 🟩 🟩 
permission-check 13s 🟩 🟩 🟩 
rust-move-tests 12s ⬜ 
Backport PR 10s 🟥 🟥 
permission-check 9s 🟩 🟩 🟩 
permission-check 9s 🟩 🟩 🟩 
permission-check 9s 🟩 🟩 🟩 
permission-check 6s 🟩 🟩 
determine-docker-build-metadata 5s 🟩 🟩 
permission-check 5s 🟩 🟩 

🚨 3 jobs on the last run were significantly faster/slower than expected

Job Duration vs 7d avg Delta
execution-performance / test-target-determinator 8m 5m +55%
test-target-determinator 8m 5m +49%
execution-performance / single-node-performance 26m 19m +37%

settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io

@vusirikala vusirikala requested review from sitalkedia and danielxiangzl and removed request for gregnazario, JoshLind and sasha8 September 15, 2024 00:15
@vusirikala vusirikala added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Sep 15, 2024
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
danielxiangzl
danielxiangzl approved these changes Sep 18, 2024
View reviewed changes
Copy link
Contributor

@danielxiangzl danielxiangzl left a comment

Choose a reason for hiding this comment

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

lgtm

@vusirikala vusirikala enabled auto-merge (squash) September 18, 2024 18:08
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions
Copy link
Contributor

✅ Forge suite framework_upgrade success on 70806ff543496aa6de7807feff49e7e1370efd20 ==> c906718266c33c05776b4e416d96226412e5f002

Compatibility test results for 70806ff543496aa6de7807feff49e7e1370efd20 ==> c906718266c33c05776b4e416d96226412e5f002 (PR)
Upgrade the nodes to version: c906718266c33c05776b4e416d96226412e5f002
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 960.44 txn/s, submitted: 962.45 txn/s, failed submission: 2.01 txn/s, expired: 2.01 txn/s, latency: 3382.19 ms, (p50: 2100 ms, p70: 3900, p90: 8100 ms, p99: 10200 ms), latency samples: 85980
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 760.80 txn/s, submitted: 763.01 txn/s, failed submission: 2.21 txn/s, expired: 2.21 txn/s, latency: 3820.28 ms, (p50: 2100 ms, p70: 4700, p90: 8400 ms, p99: 10600 ms), latency samples: 68780
5. check swarm health
Compatibility test for 70806ff543496aa6de7807feff49e7e1370efd20 ==> c906718266c33c05776b4e416d96226412e5f002 passed
Upgrade the remaining nodes to version: c906718266c33c05776b4e416d96226412e5f002
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1070.56 txn/s, submitted: 1073.48 txn/s, failed submission: 2.92 txn/s, expired: 2.92 txn/s, latency: 2859.29 ms, (p50: 2400 ms, p70: 3000, p90: 5100 ms, p99: 6400 ms), latency samples: 95380
Test Ok

@github-actions GitHub Actions
Copy link
Contributor

✅ Forge suite compat success on 70806ff543496aa6de7807feff49e7e1370efd20 ==> c906718266c33c05776b4e416d96226412e5f002

Compatibility test results for 70806ff543496aa6de7807feff49e7e1370efd20 ==> c906718266c33c05776b4e416d96226412e5f002 (PR)
1. Check liveness of validators at old version: 70806ff543496aa6de7807feff49e7e1370efd20
compatibility::simple-validator-upgrade::liveness-check : committed: 14197.59 txn/s, latency: 2418.58 ms, (p50: 2100 ms, p70: 2200, p90: 2700 ms, p99: 8100 ms), latency samples: 521400
2. Upgrading first Validator to new version: c906718266c33c05776b4e416d96226412e5f002
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7415.73 txn/s, latency: 3809.80 ms, (p50: 4300 ms, p70: 4500, p90: 4600 ms, p99: 4700 ms), latency samples: 138580
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 7260.90 txn/s, latency: 4376.62 ms, (p50: 4700 ms, p70: 4800, p90: 6000 ms, p99: 6500 ms), latency samples: 242620
3. Upgrading rest of first batch to new version: c906718266c33c05776b4e416d96226412e5f002
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7712.64 txn/s, latency: 3732.25 ms, (p50: 4100 ms, p70: 4300, p90: 4400 ms, p99: 4500 ms), latency samples: 144680
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6907.37 txn/s, latency: 4580.30 ms, (p50: 4500 ms, p70: 4600, p90: 6800 ms, p99: 7100 ms), latency samples: 262600
4. upgrading second batch to new version: c906718266c33c05776b4e416d96226412e5f002
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 11729.87 txn/s, latency: 2347.35 ms, (p50: 2600 ms, p70: 2700, p90: 2900 ms, p99: 3000 ms), latency samples: 203280
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 10012.89 txn/s, latency: 2858.93 ms, (p50: 2700 ms, p70: 2800, p90: 3000 ms, p99: 6400 ms), latency samples: 381700
5. check swarm health
Compatibility test for 70806ff543496aa6de7807feff49e7e1370efd20 ==> c906718266c33c05776b4e416d96226412e5f002 passed
Test Ok

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions
Copy link
Contributor

✅ Forge suite realistic_env_max_load success on c906718266c33c05776b4e416d96226412e5f002

two traffics test: inner traffic : committed: 12212.92 txn/s, submitted: 12213.34 txn/s, expired: 0.42 txn/s, latency: 3251.66 ms, (p50: 2700 ms, p70: 3000, p90: 3900 ms, p99: 12900 ms), latency samples: 4643640
two traffics test : committed: 99.96 txn/s, latency: 2446.88 ms, (p50: 2100 ms, p70: 2300, p90: 2700 ms, p99: 9000 ms), latency samples: 1800
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.230, avg: 0.217", "QsPosToProposal: max: 0.887, avg: 0.631", "ConsensusProposalToOrdered: max: 0.337, avg: 0.317", "ConsensusOrderedToCommit: max: 0.449, avg: 0.414", "ConsensusProposalToCommit: max: 0.782, avg: 0.731"]
Max non-epoch-change gap was: 1 rounds at version 2875528 (avg 0.00) [limit 4], 3.45s no progress at version 2875528 (avg 0.23s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.41s no progress at version 1675994 (avg 7.71s) [limit 15].
Test Ok

@vusirikala vusirikala merged commit 30f76bf into main Sep 18, 2024
134 of 143 checks passed
@vusirikala vusirikala deleted the satya/deprecate_delayed_qc_msg branch September 18, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielxiangzl danielxiangzl danielxiangzl approved these changes

@zekun000 zekun000 zekun000 approved these changes

@ibalajiarun ibalajiarun Awaiting requested review from ibalajiarun ibalajiarun is a code owner

@sitalkedia sitalkedia Awaiting requested review from sitalkedia

@igor-aptos igor-aptos Awaiting requested review from igor-aptos

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vusirikala @zekun000 @danielxiangzl