[Indexer-Grpc-V2] Add GrpcManagerService. #15726

grao1991 · 2025-01-14T01:58:24Z

Description

How Has This Been Tested?

Key Areas to Review

Type of Change

Which Components or Systems Does This Change Impact?

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 · 2025-01-14T01:58:27Z

⏱️ 1h 39m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
check-dynamic-deps	27m	🟩 🟩 🟩 🟩 🟩 (+6 more)
rust-cargo-deny	19m	🟩 🟩 🟩 🟩 🟩 (+6 more)
execution-performance / test-target-determinator	12m	🟩 🟩
test-target-determinator	10m	🟩 🟩
rust-doc-tests	7m	🟩
rust-doc-tests	6m	🟩
general-lints	5m	🟩 🟩 🟩 🟩 🟩 (+6 more)
semgrep/ci	5m	🟩 🟩 🟩 🟩 🟩 (+6 more)
fetch-last-released-docker-image-tag	3m	🟩 🟩
rust-targeted-unit-tests	3m	🟥
file_change_determinator	2m	🟩 🟩 🟩 🟩 🟩 (+6 more)
permission-check	31s	🟩 🟩 🟩 🟩 🟩 (+6 more)
permission-check	30s	🟩 🟩 🟩 🟩 🟩 (+6 more)
file_change_determinator	22s	🟩 🟩
execution-performance / single-node-performance	20s	🟩 🟩

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

Job	Duration	vs 7d avg	Delta
execution-performance / test-target-determinator	7m	5m
execution-performance / single-node-performance	10s	18m

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

graphite-app · 2025-01-14T02:14:37Z

ecosystem/indexer-grpc/indexer-grpc-manager/src/service.rs

+    ) -> Result<Response<TransactionsResponse>, Status> {
+        let request = request.into_inner();
+        let transactions = vec![];
+        // TODO(grao): Implement.,


There's a stray comma after Implement in the TODO comment.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-01-14T02:44:58Z

ecosystem/indexer-grpc/indexer-grpc-manager/src/config.rs

+pub struct IndexerGrpcManagerConfig {
+    pub(crate) chain_id: u64,
+    pub(crate) service_config: ServiceConfig,
+}


The IndexerGrpcManagerConfig struct is missing several fields that are referenced in GrpcManager::new():

self_advertised_address

grpc_manager_addresses

fullnode_addresses

These fields need to be added to maintain consistency between the config struct and its usage. The current implementation will fail to compile since these fields are required by the manager initialization.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-01-14T02:45:00Z

ecosystem/indexer-grpc/indexer-grpc-manager/src/grpc_manager.rs

+        tokio_scoped::scope(|s| {
+            s.spawn(async move {
+                self.metadata_manager.start().await.unwrap();
+            });
+            s.spawn(async move {
+                info!("Starting GrpcManager at {}.", service_config.listen_address);
+                server.serve(service_config.listen_address).await.unwrap();
+            });
+        });


The tokio_scoped::scope() block is missing an .await, which means the spawned tasks will terminate immediately. Adding .await ensures the tasks continue running:

tokio_scoped::scope(|s| { // ... }).await;

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-01-14T02:47:06Z

ecosystem/indexer-grpc/indexer-grpc-manager/src/grpc_manager.rs

+        tokio_scoped::scope(|s| {
+            s.spawn(async move {
+                info!("Starting GrpcManager at {}.", service_config.listen_address);
+                server.serve(service_config.listen_address).await.unwrap();
+            });
+        });


Using tokio_scoped::scope here will cause the server to terminate immediately since the scope exits when the spawned task is created. To keep the server running, either use tokio::spawn directly or implement a mechanism to keep the scope alive (e.g. through a shutdown signal handler). The current implementation will not serve any requests.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

graphite-app · 2025-01-14T18:51:02Z

ecosystem/indexer-grpc/indexer-grpc-manager/src/service.rs

+        let _request = request.into_inner();
+        let transactions = vec![];
+        // TODO(grao): Implement.


Since this method is not yet implemented, returning an empty vector could silently mask errors for clients. Consider returning Status::unimplemented() until the full implementation is ready. This makes the incomplete state explicit rather than implying the request succeeded with no data.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

sitalkedia · 2025-01-14T20:02:26Z

ecosystem/indexer-grpc/indexer-grpc-manager/src/grpc_manager.rs

+}
+
+impl GrpcManager {
+    pub(crate) async fn new(config: &IndexerGrpcManagerConfig) -> Self {


Why does this need to be async?

larry-aptos · 2025-01-15T20:40:37Z

ecosystem/indexer-grpc/indexer-grpc-manager/src/config.rs

+static GRPC_MANAGER: OnceCell<GrpcManager> = OnceCell::const_new();
+
+#[derive(Clone, Debug, Deserialize, Serialize)]
+pub(crate) struct ServiceConfig {


do we want to have a custom health check endpoint so that k8s can kick-in to restart the pod if error happens? or the design is service can restart itself.

Good point. Will come up with something later.

larry-aptos · 2025-01-15T20:43:15Z

ecosystem/indexer-grpc/indexer-grpc-manager/src/service.rs

+            // future.
+            if let Some(address) = self.pick_live_data_service(starting_version) {
+                address
+            } else if let Some(address) = self.pick_historical_data_service(starting_version).await {


github-actions · 2025-01-16T01:33:53Z

✅ Forge suite `compat` success on `f69dc31b0f674662b6bfa941dc7eee9ce9035e29` ==> `2acf36bb10cc2eb4c5a6b952861a1ed0a6f7d8b8`

Compatibility test results for f69dc31b0f674662b6bfa941dc7eee9ce9035e29 ==> 2acf36bb10cc2eb4c5a6b952861a1ed0a6f7d8b8 (PR)
1. Check liveness of validators at old version: f69dc31b0f674662b6bfa941dc7eee9ce9035e29
compatibility::simple-validator-upgrade::liveness-check : committed: 15382.43 txn/s, latency: 2047.72 ms, (p50: 2100 ms, p70: 2200, p90: 2500 ms, p99: 3600 ms), latency samples: 499460
2. Upgrading first Validator to new version: 2acf36bb10cc2eb4c5a6b952861a1ed0a6f7d8b8
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 5051.48 txn/s, latency: 6116.50 ms, (p50: 6900 ms, p70: 7400, p90: 7600 ms, p99: 7700 ms), latency samples: 99620
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 4985.80 txn/s, latency: 6931.73 ms, (p50: 7600 ms, p70: 7600, p90: 7700 ms, p99: 7900 ms), latency samples: 175840
3. Upgrading rest of first batch to new version: 2acf36bb10cc2eb4c5a6b952861a1ed0a6f7d8b8
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 4886.39 txn/s, latency: 6342.05 ms, (p50: 7000 ms, p70: 7700, p90: 8000 ms, p99: 8000 ms), latency samples: 97040
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 4981.86 txn/s, latency: 6908.81 ms, (p50: 7700 ms, p70: 7700, p90: 7900 ms, p99: 8000 ms), latency samples: 175760
4. upgrading second batch to new version: 2acf36bb10cc2eb4c5a6b952861a1ed0a6f7d8b8
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 8363.03 txn/s, latency: 3634.24 ms, (p50: 4200 ms, p70: 4300, p90: 4800 ms, p99: 5000 ms), latency samples: 154120
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 8119.48 txn/s, latency: 4129.67 ms, (p50: 4300 ms, p70: 4600, p90: 5000 ms, p99: 5200 ms), latency samples: 275300
5. check swarm health
Compatibility test for f69dc31b0f674662b6bfa941dc7eee9ce9035e29 ==> 2acf36bb10cc2eb4c5a6b952861a1ed0a6f7d8b8 passed
Test Ok

github-actions · 2025-01-16T01:33:57Z

✅ Forge suite `realistic_env_max_load` success on `2acf36bb10cc2eb4c5a6b952861a1ed0a6f7d8b8`

two traffics test: inner traffic : committed: 14544.78 txn/s, latency: 2731.87 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3900 ms), latency samples: 5530220
two traffics test : committed: 99.98 txn/s, latency: 1372.64 ms, (p50: 1300 ms, p70: 1400, p90: 1500 ms, p99: 2400 ms), latency samples: 1780
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.693, avg: 1.565", "ConsensusProposalToOrdered: max: 0.303, avg: 0.293", "ConsensusOrderedToCommit: max: 0.314, avg: 0.298", "ConsensusProposalToCommit: max: 0.614, avg: 0.591"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.97s no progress at version 46729 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.67s no progress at version 2921461 (avg 0.67s) [limit 16].
Test Ok

grao1991 force-pushed the grao_grpc_manager branch from 449a3b5 to cf77eeb Compare January 14, 2025 02:13

graphite-app bot reviewed Jan 14, 2025

View reviewed changes

grao1991 force-pushed the grao_grpc_manager branch from cf77eeb to 6efa576 Compare January 14, 2025 02:16

grao1991 changed the title ~~[Indexer-Grpc-V2] Add GrpcManager.~~ [Indexer-Grpc-V2] Add GrpcManagerService. Jan 14, 2025

grao1991 marked this pull request as ready for review January 14, 2025 02:17

grao1991 requested review from geekflyer, sitalkedia, sherry-x and larry-aptos January 14, 2025 02:17

grao1991 force-pushed the grao_grpc_manager branch from 6efa576 to fe81cff Compare January 14, 2025 02:18

grao1991 requested review from rtso and bowenyang007 January 14, 2025 02:18

grao1991 force-pushed the grao_grpc_manager branch from fe81cff to 0ff320c Compare January 14, 2025 02:44

graphite-app bot reviewed Jan 14, 2025

View reviewed changes

grao1991 force-pushed the grao_grpc_manager branch from 0ff320c to 28b1410 Compare January 14, 2025 02:45

graphite-app bot reviewed Jan 14, 2025

View reviewed changes

grao1991 force-pushed the grao_grpc_manager branch 3 times, most recently from 77618df to ee7c3ae Compare January 14, 2025 18:49

graphite-app bot reviewed Jan 14, 2025

View reviewed changes

sitalkedia approved these changes Jan 14, 2025

View reviewed changes

grao1991 enabled auto-merge (squash) January 15, 2025 20:35

larry-aptos approved these changes Jan 15, 2025

View reviewed changes

larry-aptos reviewed Jan 15, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

[Indexer-Grpc-V2] Add GrpcManagerService.

2acf36b

grao1991 force-pushed the grao_grpc_manager branch from ee7c3ae to 2acf36b Compare January 16, 2025 01:07

This comment has been minimized.

Sign in to view

grao1991 merged commit 7c89297 into main Jan 16, 2025
46 checks passed

grao1991 deleted the grao_grpc_manager branch January 16, 2025 01:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Indexer-Grpc-V2] Add GrpcManagerService. #15726

[Indexer-Grpc-V2] Add GrpcManagerService. #15726

grao1991 commented Jan 14, 2025 •

edited

Loading

trunk-io bot commented Jan 14, 2025 •

edited

Loading

graphite-app bot Jan 14, 2025

graphite-app bot Jan 14, 2025

graphite-app bot Jan 14, 2025

graphite-app bot Jan 14, 2025

graphite-app bot Jan 14, 2025

sitalkedia Jan 14, 2025

larry-aptos Jan 15, 2025 •

edited

Loading

grao1991 Jan 16, 2025

larry-aptos Jan 15, 2025

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 16, 2025

github-actions bot commented Jan 16, 2025

[Indexer-Grpc-V2] Add GrpcManagerService. #15726

[Indexer-Grpc-V2] Add GrpcManagerService. #15726

Conversation

grao1991 commented Jan 14, 2025 • edited Loading

Description

How Has This Been Tested?

Key Areas to Review

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

trunk-io bot commented Jan 14, 2025 • edited Loading

graphite-app bot Jan 14, 2025

Choose a reason for hiding this comment

graphite-app bot Jan 14, 2025

Choose a reason for hiding this comment

graphite-app bot Jan 14, 2025

Choose a reason for hiding this comment

graphite-app bot Jan 14, 2025

Choose a reason for hiding this comment

graphite-app bot Jan 14, 2025

Choose a reason for hiding this comment

sitalkedia Jan 14, 2025

Choose a reason for hiding this comment

larry-aptos Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

grao1991 Jan 16, 2025

Choose a reason for hiding this comment

larry-aptos Jan 15, 2025

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 16, 2025

✅ Forge suite compat success on f69dc31b0f674662b6bfa941dc7eee9ce9035e29 ==> 2acf36bb10cc2eb4c5a6b952861a1ed0a6f7d8b8

github-actions bot commented Jan 16, 2025

✅ Forge suite realistic_env_max_load success on 2acf36bb10cc2eb4c5a6b952861a1ed0a6f7d8b8

grao1991 commented Jan 14, 2025 •

edited

Loading

trunk-io bot commented Jan 14, 2025 •

edited

Loading

larry-aptos Jan 15, 2025 •

edited

Loading

✅ Forge suite `compat` success on `f69dc31b0f674662b6bfa941dc7eee9ce9035e29` ==> `2acf36bb10cc2eb4c5a6b952861a1ed0a6f7d8b8`

✅ Forge suite `realistic_env_max_load` success on `2acf36bb10cc2eb4c5a6b952861a1ed0a6f7d8b8`