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

[quorum store] constrain txn pull size to sender_max_total_txns #12532

Merged
merged 2 commits into from
Mar 14, 2024
Merged

[quorum store] constrain txn pull size to sender_max_total_txns #12532

merged 2 commits into from
Mar 14, 2024

Conversation

bchocho
Copy link
Contributor

@bchocho bchocho commented Mar 14, 2024 •
edited
Loading

Description

We were not constraining on the sender side, even though we check on the receiver side. It's an oversight. We hadn't run into this previously because the backpressure configs meant we never tried to pull more than the sender/receiver side limit.

Type of Change

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

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?

Existing tests

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 Mar 14, 2024 •
edited
Loading

⏱️ 7h 12m total CI duration on this PR
Job Cumulative Duration Recent Runs
windows-build 1h 30m 🟩 🟩 🟩 🟩 
rust-unit-tests 1h 17m 🟩 🟩 🟩 
rust-smoke-tests 59m ⬜ 🟩 
execution-performance / single-node-performance 44m 🟩 🟩 
rust-images / rust-all 27m 🟩 🟩 
forge-compat-test / forge 27m 🟩 🟩 
forge-e2e-test / forge 26m 🟥 🟩 
rust-lints 24m 🟩 🟩 🟩 
cli-e2e-tests / run-cli-tests 14m 🟩 🟩 
check 13m 🟩 🟩 ⬜ 🟩 
run-tests-main-branch 11m 🟥 🟥 🟥 
check-dynamic-deps 7m 🟩 🟩 🟩 🟩 
general-lints 6m 🟩 🟩 🟩 
node-api-compatibility-tests / node-api-compatibility-tests 2m 🟩 🟩 
semgrep/ci 2m 🟩 🟩 🟩 🟩 
file_change_determinator 44s 🟩 🟩 🟩 🟩 
file_change_determinator 38s 🟩 🟩 ⬜ 🟩 
file_change_determinator 24s 🟩 🟩 
execution-performance / file_change_determinator 21s 🟩 🟩 
permission-check 14s 🟩 🟩 🟩 🟩 
permission-check 12s 🟩 🟩 🟩 🟩 
permission-check 11s 🟩 🟩 🟩 🟩 
permission-check 11s 🟩 🟩 🟩 🟩 
permission-check 7s 🟩 🟩 
determine-docker-build-metadata 5s 🟩 🟩 

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

@bchocho bchocho marked this pull request as ready for review March 14, 2024 18:28
@bchocho bchocho requested review from sasha8 and gelash as code owners March 14, 2024 18:28
@bchocho bchocho enabled auto-merge (squash) March 14, 2024 21:05
@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
@bchocho bchocho changed the title [quorum store] constrain txn pull size to sender_max_batch_txns [quorum store] constrain txn pull size to sender_max_total_txns Mar 14, 2024
@bchocho bchocho disabled auto-merge March 14, 2024 21:35
@bchocho bchocho enabled auto-merge (squash) March 14, 2024 21:36
@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 realistic_env_max_load success on 7eedd39c1ef91cb11a00da29180880af46cabfea

two traffics test: inner traffic : committed: 8048 txn/s, latency: 4870 ms, (p50: 4600 ms, p90: 5700 ms, p99: 9900 ms), latency samples: 3476880
two traffics test : committed: 100 txn/s, latency: 1857 ms, (p50: 1800 ms, p90: 2100 ms, p99: 2400 ms), latency samples: 1800
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.249, avg: 0.207", "QsPosToProposal: max: 0.292, avg: 0.261", "ConsensusProposalToOrdered: max: 0.447, avg: 0.407", "ConsensusOrderedToCommit: max: 0.317, avg: 0.304", "ConsensusProposalToCommit: max: 0.730, avg: 0.711"]
Max round gap was 1 [limit 4] at version 1662982. Max no progress secs was 4.767919 [limit 15] at version 1662982.
Test Ok

@github-actions GitHub Actions
Copy link
Contributor

✅ Forge suite compat success on aptos-node-v1.9.5 ==> 7eedd39c1ef91cb11a00da29180880af46cabfea

Compatibility test results for aptos-node-v1.9.5 ==> 7eedd39c1ef91cb11a00da29180880af46cabfea (PR)
1. Check liveness of validators at old version: aptos-node-v1.9.5
compatibility::simple-validator-upgrade::liveness-check : committed: 5798 txn/s, latency: 4792 ms, (p50: 4800 ms, p90: 7500 ms, p99: 8400 ms), latency samples: 243520
2. Upgrading first Validator to new version: 7eedd39c1ef91cb11a00da29180880af46cabfea
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1227 txn/s, latency: 23249 ms, (p50: 24400 ms, p90: 30800 ms, p99: 31800 ms), latency samples: 65060
3. Upgrading rest of first batch to new version: 7eedd39c1ef91cb11a00da29180880af46cabfea
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 419 txn/s, submitted: 632 txn/s, expired: 212 txn/s, latency: 39075 ms, (p50: 36800 ms, p90: 58700 ms, p99: 59900 ms), latency samples: 27709
4. upgrading second batch to new version: 7eedd39c1ef91cb11a00da29180880af46cabfea
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 2340 txn/s, latency: 12329 ms, (p50: 12600 ms, p90: 17700 ms, p99: 18900 ms), latency samples: 112360
5. check swarm health
Compatibility test for aptos-node-v1.9.5 ==> 7eedd39c1ef91cb11a00da29180880af46cabfea passed
Test Ok

@bchocho bchocho merged commit 9f98674 into main Mar 14, 2024
74 of 80 checks passed
@bchocho bchocho deleted the brian/qs-sender-limit branch March 14, 2024 22:09
igor-aptos pushed a commit that referenced this pull request Mar 15, 2024
@bchocho @igor-aptos
[quorum store] constrain txn pull size to sender_max_total_txns (#12532)
5574632 
## Description

We were not constraining on the sender side, even though we check on the receiver side. It's an oversight. We hadn't run into this previously because the backpressure configs meant we never tried to pull more than the sender/receiver side limit.
igor-aptos added a commit that referenced this pull request Mar 19, 2024
@igor-aptos
[quorum store] constrain txn pull size to sender_max_total_txns (#12532…
a4353ea 
…) (#12550)

## Description

We were not constraining on the sender side, even though we check on the receiver side. It's an oversight. We hadn't run into this previously because the backpressure configs meant we never tried to pull more than the sender/receiver side limit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igor-aptos igor-aptos igor-aptos approved these changes

@vusirikala vusirikala vusirikala approved these changes

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

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

@zekun000 zekun000 Awaiting requested review from zekun000

@ibalajiarun ibalajiarun Awaiting requested review from ibalajiarun

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bchocho @vusirikala @igor-aptos