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

[CLI] Update processor dependency #12568

Merged
merged 1 commit into from
Mar 21, 2024
Merged

[CLI] Update processor dependency #12568

merged 1 commit into from
Mar 21, 2024

Conversation

banool
Copy link
Contributor

@banool banool commented Mar 18, 2024

Description

This update required more than just bumping the rev, a few things changed:

  • To ensure we don't onboard the libpq dep, we set default-features = false. See more here: Add support for using diesel async for migrations to avoid pq-sys dep aptos-indexer-processors#325.
  • In the new version of the library run_pending_migrations takes in a generic DB object. To support this, run_migrations was updated to use AsyncConnectionWrapper since async connection itself doesn't impl the relevant traits. This required some changes to how we run migrations on our side.
  • There is a new processor, TransactionMetadataProcessor. We add support for it here and enable it by default.
  • Some processors (those with lookups) now take additional configuration.
  • Some deps were updated in aptos-indexer-processors, we make necessary updates in aptos-core accordingly.

This PR has a secondary goal of making sure we're using an in-tree rev to avoid this problem: rust-lang/cargo#13555. The rev we depend on is in main of aptos-indexer-processors, it doesn't come from a branch.

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?

Run a local testnet:

cargo run -p aptos -- node run-local-testnet --force-restart --with-indexer-api --assume-yes

Run the TS SDK v2 tests against it:

pnpm test

Besides that, just CI.

Key Areas to Review

Ensure updating other deps won't cause issues.

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

Copy link

trunk-io bot commented Mar 18, 2024

⏱️ 7h 2m total CI duration on this PR
Job Cumulative Duration Recent Runs
windows-build 2h 4m 🟥🟩🟩🟩🟩
rust-unit-tests 1h 18m 🟥🟩🟩
rust-smoke-tests 38m 🟩
rust-lints 34m 🟥🟩🟩
run-tests-main-branch 25m 🟩🟩🟩🟩🟩 (+1 more)
execution-performance / single-node-performance 24m 🟩
rust-images / rust-all 21m 🟩
check 17m 🟩🟩🟩🟩
forge-e2e-test / forge 16m 🟩
forge-compat-test / forge 12m 🟩
check-dynamic-deps 10m 🟥🟩🟩🟩🟩
general-lints 9m 🟩🟩🟩🟩
cli-e2e-tests / run-cli-tests 6m 🟩
semgrep/ci 2m 🟩🟩🟩🟩🟩
file_change_determinator 1m 🟩🟩🟩🟩🟩 (+1 more)
node-api-compatibility-tests / node-api-compatibility-tests 54s 🟩
file_change_determinator 53s 🟩🟩🟩🟩🟩
permission-check 31s 🟩🟩🟩🟩🟩 (+1 more)
permission-check 29s 🟩🟩🟩🟩🟩 (+1 more)
permission-check 20s 🟩🟩🟩🟩🟩 (+1 more)
permission-check 19s 🟩🟩🟩🟩🟩 (+1 more)
execution-performance / file_change_determinator 18s 🟩
file_change_determinator 12s 🟩
determine-docker-build-metadata 4s 🟩
permission-check 3s 🟩

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

Job Duration vs 7d avg Delta
windows-build 30m 21m +44%
rust-images / rust-all 21m 15m +40%

settingsfeedbackdocs ⋅ learn more about trunk.io

Base automatically changed from banool/cli-self-update-cross-fs to main March 18, 2024 16:50
@banool
Copy link
Contributor Author

banool commented Mar 19, 2024

Blocked on aptos-labs/aptos-indexer-processors#325.

Update: Ready to roll.

@banool banool force-pushed the banool/processor-dep-update branch 2 times, most recently from 515b2fe to c2ca38f Compare March 21, 2024 08:40
@banool banool changed the title Update processor dep for CLI, update aptos-core deps accordingly [CLI] Update processor dependency Mar 21, 2024
@banool banool force-pushed the banool/processor-dep-update branch from c2ca38f to c1d5f33 Compare March 21, 2024 08:40
@banool banool force-pushed the banool/processor-dep-update branch from c1d5f33 to 5964a81 Compare March 21, 2024 18:06
@banool banool marked this pull request as ready for review March 21, 2024 18:07
@banool banool requested a review from gregnazario as a code owner March 21, 2024 18:07
@banool banool enabled auto-merge (squash) March 21, 2024 18:13

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite compat success on aptos-node-v1.9.5 ==> 5964a8116224ac49403cd2fe48b8debff0154b56

Compatibility test results for aptos-node-v1.9.5 ==> 5964a8116224ac49403cd2fe48b8debff0154b56 (PR)
1. Check liveness of validators at old version: aptos-node-v1.9.5
compatibility::simple-validator-upgrade::liveness-check : committed: 5616 txn/s, latency: 5817 ms, (p50: 5400 ms, p90: 8100 ms, p99: 22600 ms), latency samples: 196560
2. Upgrading first Validator to new version: 5964a8116224ac49403cd2fe48b8debff0154b56
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1106 txn/s, latency: 25935 ms, (p50: 28000 ms, p90: 37100 ms, p99: 38000 ms), latency samples: 57540
3. Upgrading rest of first batch to new version: 5964a8116224ac49403cd2fe48b8debff0154b56
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 394 txn/s, submitted: 588 txn/s, expired: 193 txn/s, latency: 34743 ms, (p50: 33700 ms, p90: 55200 ms, p99: 86900 ms), latency samples: 37894
4. upgrading second batch to new version: 5964a8116224ac49403cd2fe48b8debff0154b56
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 1863 txn/s, latency: 15048 ms, (p50: 16200 ms, p90: 20100 ms, p99: 21400 ms), latency samples: 95040
5. check swarm health
Compatibility test for aptos-node-v1.9.5 ==> 5964a8116224ac49403cd2fe48b8debff0154b56 passed
Test Ok

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on 5964a8116224ac49403cd2fe48b8debff0154b56

two traffics test: inner traffic : committed: 7748 txn/s, latency: 5062 ms, (p50: 4800 ms, p90: 5900 ms, p99: 13500 ms), latency samples: 3347340
two traffics test : committed: 100 txn/s, latency: 2029 ms, (p50: 1800 ms, p90: 2200 ms, p99: 7100 ms), latency samples: 1740
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.213, avg: 0.203", "QsPosToProposal: max: 0.360, avg: 0.268", "ConsensusProposalToOrdered: max: 0.478, avg: 0.435", "ConsensusOrderedToCommit: max: 0.319, avg: 0.301", "ConsensusProposalToCommit: max: 0.788, avg: 0.736"]
Max round gap was 1 [limit 4] at version 1684368. Max no progress secs was 6.129437 [limit 15] at version 1684368.
Test Ok

@banool banool merged commit a63d034 into main Mar 21, 2024
80 of 82 checks passed
@banool banool deleted the banool/processor-dep-update branch March 21, 2024 19:48
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.

3 participants