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

[Indexer-Grpc-V2] Add DataManager. #15784

Merged
merged 1 commit into from
Jan 22, 2025
Merged

[Indexer-Grpc-V2] Add DataManager. #15784

merged 1 commit into from
Jan 22, 2025
+452 −10

Conversation

grao1991
Copy link
Contributor

Description

How Has This Been Tested?

Key Areas to Review

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
  • Move Compiler
  • Other (specify)

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 Jan 21, 2025 •
edited
Loading

⏱️ 3h 31m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
execution-performance / single-node-performance 1h 40m 🟩 🟩 🟩 🟩 
execution-performance / test-target-determinator 20m 🟩 🟩 🟩 🟩 
test-target-determinator 19m ⬜ 🟩 🟩 🟩 
forge-e2e-test / forge 14m 🟩 
check-dynamic-deps 13m 🟩 🟩 🟩 🟩 🟩 (+1 more)
rust-cargo-deny 10m 🟩 🟩 🟩 🟩 🟩 (+1 more)
fetch-last-released-docker-image-tag 6m 🟩 🟩 🟩 🟩 
rust-doc-tests 5m 🟩 
rust-doc-tests 5m 🟩 
rust-doc-tests 5m 🟩 
rust-doc-tests 4m ⬜ 
general-lints 3m 🟩 🟩 🟩 🟩 🟩 (+1 more)
semgrep/ci 3m 🟩 🟩 🟩 🟩 🟩 (+1 more)
file_change_determinator 1m 🟩 🟩 🟩 🟩 🟩 (+1 more)
file_change_determinator 44s 🟩 🟩 🟩 🟩 

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

Job Duration vs 7d avg Delta
execution-performance / single-node-performance 26m 16m +59%

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

larry-aptos
larry-aptos approved these changes Jan 22, 2025
View reviewed changes
ecosystem/indexer-grpc/indexer-grpc-manager/src/config.rs
pub(crate) self_advertised_address: GrpcAddress,
pub(crate) grpc_manager_addresses: Vec<GrpcAddress>,
pub(crate) fullnode_addresses: Vec<GrpcAddress>,
}

const fn default_cache_config() -> CacheConfig {
Copy link
Contributor

Choose a reason for hiding this comment

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

why this over impl Default trait?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will serde accept that?

Copy link
Contributor

Choose a reason for hiding this comment

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

i thought something like #[serde(default)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am using this on line 32? Isn't this what you mean?

ecosystem/indexer-grpc/indexer-grpc-manager/src/data_manager.rs Outdated
}
}

fn maybe_evict(&mut self) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: comments.

only when the file store is delayed, we'll not evict entries.

Copy link
Contributor

Choose a reason for hiding this comment

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

also based on the name, it could be entried are evicted but the cache is still no usable... rename it? cleanup or garbage_collect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@grao1991 grao1991 enabled auto-merge (squash) January 22, 2025 00:45
larry-aptos
larry-aptos reviewed Jan 22, 2025
View reviewed changes
ecosystem/indexer-grpc/indexer-grpc-manager/src/data_manager.rs
};
drop(cache);

debug!(
Copy link
Contributor

Choose a reason for hiding this comment

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

if you want to surface these logs up in humio, you may upgrade to info or higher; I was under the impression that only info and above will be collected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I choose debug! for this one on purpose. Feels too verbose for info!. I will have enough information on dashboard for version.

@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

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 compat success on bb609ba7417025d8af1e199c345631c8b4b2c412 ==> dff70f4f90a0490254c37eeee9ac533d855ae901

Compatibility test results for bb609ba7417025d8af1e199c345631c8b4b2c412 ==> dff70f4f90a0490254c37eeee9ac533d855ae901 (PR)
1. Check liveness of validators at old version: bb609ba7417025d8af1e199c345631c8b4b2c412
compatibility::simple-validator-upgrade::liveness-check : committed: 11508.60 txn/s, latency: 2707.59 ms, (p50: 2300 ms, p70: 2800, p90: 4000 ms, p99: 10200 ms), latency samples: 382300
2. Upgrading first Validator to new version: dff70f4f90a0490254c37eeee9ac533d855ae901
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 3929.79 txn/s, latency: 7910.19 ms, (p50: 8800 ms, p70: 9300, p90: 9900 ms, p99: 10000 ms), latency samples: 89020
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 4027.15 txn/s, latency: 8474.04 ms, (p50: 9400 ms, p70: 9600, p90: 9900 ms, p99: 10000 ms), latency samples: 141920
3. Upgrading rest of first batch to new version: dff70f4f90a0490254c37eeee9ac533d855ae901
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 3825.63 txn/s, latency: 8100.24 ms, (p50: 8900 ms, p70: 9500, p90: 9900 ms, p99: 9900 ms), latency samples: 86880
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 3847.62 txn/s, latency: 8894.44 ms, (p50: 9800 ms, p70: 10000, p90: 10200 ms, p99: 10300 ms), latency samples: 136520
4. upgrading second batch to new version: dff70f4f90a0490254c37eeee9ac533d855ae901
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 7557.82 txn/s, latency: 4026.78 ms, (p50: 4500 ms, p70: 4800, p90: 5500 ms, p99: 5800 ms), latency samples: 139880
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 7560.52 txn/s, latency: 4534.34 ms, (p50: 4600 ms, p70: 5400, p90: 5700 ms, p99: 5900 ms), latency samples: 253320
5. check swarm health
Compatibility test for bb609ba7417025d8af1e199c345631c8b4b2c412 ==> dff70f4f90a0490254c37eeee9ac533d855ae901 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 dff70f4f90a0490254c37eeee9ac533d855ae901

two traffics test: inner traffic : committed: 14631.68 txn/s, submitted: 14638.31 txn/s, expired: 6.63 txn/s, latency: 2688.31 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3300 ms), latency samples: 5563280
two traffics test : committed: 99.98 txn/s, latency: 1457.49 ms, (p50: 1400 ms, p70: 1500, p90: 1500 ms, p99: 1900 ms), latency samples: 1720
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.538, avg: 1.464", "ConsensusProposalToOrdered: max: 0.296, avg: 0.293", "ConsensusOrderedToCommit: max: 0.414, avg: 0.403", "ConsensusProposalToCommit: max: 0.708, avg: 0.696"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.60s no progress at version 46814 (avg 0.19s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.64s no progress at version 6258526 (avg 0.63s) [limit 16].
Test Ok

@grao1991 grao1991 merged commit 2dc2617 into main Jan 22, 2025
43 of 46 checks passed
@grao1991 grao1991 deleted the grao_data_manager branch January 22, 2025 03:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@larry-aptos larry-aptos larry-aptos approved these changes

@msmouse msmouse msmouse approved these changes

@sitalkedia sitalkedia Awaiting requested review from sitalkedia

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
@grao1991 @msmouse @larry-aptos