[Indexer-Grpc-V2] Add FileStoreOperatorV2. #15779

grao1991 · 2025-01-21T08:31: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-21T08:31:27Z

⏱️ 2h 34m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
execution-performance / single-node-performance	1h 11m	🟩 🟩 🟩
test-target-determinator	14m	🟩 🟩 🟩
execution-performance / test-target-determinator	14m	🟩 🟩 🟩
check-dynamic-deps	14m	🟩 🟩 🟩 🟩 🟩 (+1 more)
rust-cargo-deny	9m	🟩 🟩 🟩 🟩 🟩
permission-check	5m	🟩 🟩 🟩 🟩 🟩 (+1 more)
rust-doc-tests	5m	🟩
rust-doc-tests	5m	🟩
rust-doc-tests	5m	🟩
fetch-last-released-docker-image-tag	5m	🟩 🟩 🟩
general-lints	3m	🟩 🟩 🟩 🟩 🟩
semgrep/ci	2m	🟩 🟩 🟩 🟩 🟩 (+1 more)
file_change_determinator	59s	🟩 🟩 🟩 🟩 🟩
file_change_determinator	37s	🟩 🟩 🟩
permission-check	14s	🟩 🟩 🟩 🟩 🟩 (+1 more)

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

ecosystem/indexer-grpc/indexer-grpc-utils/src/file_store_operator_v2/file_store_operator.rs

larry-aptos

overall looks good; some nits.

larry-aptos · 2025-01-21T19:53:13Z

ecosystem/indexer-grpc/indexer-grpc-utils/src/file_store_operator_v2/common.rs

+#[derive(Serialize, Deserialize, Default, Clone)]
+pub struct BatchMetadata {
+    // "[first_version, last_version), size_bytes"
+    pub files: Vec<(u64, u64, usize)>,


let's name these fields; readers are required to read this comment to understand these numbers.

larry-aptos · 2025-01-21T20:01:46Z

ecosystem/indexer-grpc/indexer-grpc-utils/src/file_store_operator_v2/local.rs

+            .unwrap()
+            .next_entry()
+            .await
+            .unwrap()


for the first unwrap, can we change to expect and include path information in the message?

for the second unwrap, can we include something like IO error happens for path?

hmm, why not check the existence of the metadata file directly? or what does is_initialized mean?

It checks if the folder is empty..

Trying to capture the case when the folder is not empty, but metadata is disappeared for any reason (which likely indicates data corruption). In that case I'd like to treat it as an error rather than re-initialize it.

larry-aptos · 2025-01-21T20:04:10Z

ecosystem/indexer-grpc/indexer-grpc-utils/src/file_store_operator_v2/local.rs

+            "Verifying the path exists for LocalFileStore."
+        );
+        if !path.exists() {
+            panic!("LocalFileStore path does not exist.");


nit: include the path in the error message.

The path is printed out right above.

larry-aptos · 2025-01-21T20:08:20Z

ecosystem/indexer-grpc/indexer-grpc-utils/src/file_store_operator_v2/gcs.rs

+            ..Default::default()
+        };
+
+        let response = Object::list(&self.bucket_name, request)


larry-aptos · 2025-01-21T21:04:34Z

ecosystem/indexer-grpc/indexer-grpc-utils/src/file_store_operator_v2/common.rs

+    /// The tag of the store, for logging.
+    fn tag(&self) -> &str;
+
+    async fn is_initialized(&self) -> bool;


need comments: what does true and false stand for?

larry-aptos · 2025-01-21T21:25:10Z

ecosystem/indexer-grpc/indexer-grpc-utils/src/file_store_operator_v2/file_store_reader.rs

+            end_file_index = end_file_index.min(file_index.saturating_add(max_files));
+        }
+
+        for i in file_index..end_file_index {


as a follow-up, let's add unit tests for this logic.

github-actions · 2025-01-21T22:28:11Z

✅ Forge suite `compat` success on `bb609ba7417025d8af1e199c345631c8b4b2c412` ==> `42f5a779eada739e117a9d4873e838afb98ccb9f`

Compatibility test results for bb609ba7417025d8af1e199c345631c8b4b2c412 ==> 42f5a779eada739e117a9d4873e838afb98ccb9f (PR)
1. Check liveness of validators at old version: bb609ba7417025d8af1e199c345631c8b4b2c412
compatibility::simple-validator-upgrade::liveness-check : committed: 10887.33 txn/s, latency: 2890.53 ms, (p50: 2300 ms, p70: 3100, p90: 4200 ms, p99: 13900 ms), latency samples: 362380
2. Upgrading first Validator to new version: 42f5a779eada739e117a9d4873e838afb98ccb9f
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 4035.64 txn/s, latency: 7719.69 ms, (p50: 8500 ms, p70: 9000, p90: 9400 ms, p99: 9600 ms), latency samples: 89520
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 4068.27 txn/s, latency: 8406.47 ms, (p50: 9100 ms, p70: 9600, p90: 10000 ms, p99: 10000 ms), latency samples: 142340
3. Upgrading rest of first batch to new version: 42f5a779eada739e117a9d4873e838afb98ccb9f
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 3060.14 txn/s, latency: 9606.71 ms, (p50: 10200 ms, p70: 11500, p90: 12500 ms, p99: 12700 ms), latency samples: 68960
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 3157.57 txn/s, latency: 10837.12 ms, (p50: 11600 ms, p70: 12500, p90: 12600 ms, p99: 12700 ms), latency samples: 123480
4. upgrading second batch to new version: 42f5a779eada739e117a9d4873e838afb98ccb9f
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 7831.28 txn/s, latency: 3869.63 ms, (p50: 4500 ms, p70: 4600, p90: 5000 ms, p99: 5100 ms), latency samples: 146120
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 7662.83 txn/s, latency: 4437.99 ms, (p50: 4700 ms, p70: 5000, p90: 5100 ms, p99: 5300 ms), latency samples: 259380
5. check swarm health
Compatibility test for bb609ba7417025d8af1e199c345631c8b4b2c412 ==> 42f5a779eada739e117a9d4873e838afb98ccb9f passed
Test Ok

github-actions · 2025-01-21T22:28:14Z

✅ Forge suite `realistic_env_max_load` success on `42f5a779eada739e117a9d4873e838afb98ccb9f`

two traffics test: inner traffic : committed: 14474.95 txn/s, submitted: 14481.32 txn/s, expired: 6.36 txn/s, latency: 2712.44 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3300 ms), latency samples: 5503680
two traffics test : committed: 99.99 txn/s, latency: 1486.74 ms, (p50: 1400 ms, p70: 1500, p90: 1600 ms, p99: 1800 ms), latency samples: 1740
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 1.520, avg: 1.463", "ConsensusProposalToOrdered: max: 0.296, avg: 0.294", "ConsensusOrderedToCommit: max: 0.435, avg: 0.418", "ConsensusProposalToCommit: max: 0.730, avg: 0.712"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.66s no progress at version 29480 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.68s no progress at version 2370190 (avg 0.68s) [limit 16].
Test Ok

grao1991 force-pushed the grao_file_store_operator_v2 branch from 3ddf5e5 to a9bb3d0 Compare January 21, 2025 19:26

grao1991 requested a review from larry-aptos January 21, 2025 19:28

grao1991 marked this pull request as ready for review January 21, 2025 19:28

grao1991 requested a review from sitalkedia January 21, 2025 19:28

grao1991 force-pushed the grao_file_store_operator_v2 branch from a9bb3d0 to 402b6bf Compare January 21, 2025 19:29

grao1991 enabled auto-merge (squash) January 21, 2025 19:29

larry-aptos reviewed Jan 21, 2025

View reviewed changes

ecosystem/indexer-grpc/indexer-grpc-utils/src/file_store_operator_v2/file_store_operator.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

grao1991 force-pushed the grao_file_store_operator_v2 branch from 402b6bf to 3c69c17 Compare January 21, 2025 19:47

This comment has been minimized.

Sign in to view

larry-aptos approved these changes Jan 21, 2025

View reviewed changes

[Indexer-Grpc-V2] Add FileStoreOperatorV2.

42f5a77

grao1991 force-pushed the grao_file_store_operator_v2 branch from 3c69c17 to 42f5a77 Compare January 21, 2025 22:01

This comment has been minimized.

Sign in to view

areshand approved these changes Jan 21, 2025

View reviewed changes

grao1991 merged commit cad979a into main Jan 21, 2025
46 checks passed

grao1991 deleted the grao_file_store_operator_v2 branch January 21, 2025 23:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Indexer-Grpc-V2] Add FileStoreOperatorV2. #15779

[Indexer-Grpc-V2] Add FileStoreOperatorV2. #15779

grao1991 commented Jan 21, 2025 •

edited

Loading

trunk-io bot commented Jan 21, 2025 •

edited

Loading

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.

larry-aptos left a comment

larry-aptos Jan 21, 2025

grao1991 Jan 21, 2025

larry-aptos Jan 21, 2025

larry-aptos Jan 21, 2025 •

edited

Loading

grao1991 Jan 21, 2025

larry-aptos Jan 21, 2025

grao1991 Jan 21, 2025

larry-aptos Jan 21, 2025

grao1991 Jan 21, 2025

larry-aptos Jan 21, 2025

grao1991 Jan 21, 2025

larry-aptos Jan 21, 2025

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 21, 2025

github-actions bot commented Jan 21, 2025

[Indexer-Grpc-V2] Add FileStoreOperatorV2. #15779

[Indexer-Grpc-V2] Add FileStoreOperatorV2. #15779

Conversation

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

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.

larry-aptos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larry-aptos Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jan 21, 2025

✅ Forge suite compat success on bb609ba7417025d8af1e199c345631c8b4b2c412 ==> 42f5a779eada739e117a9d4873e838afb98ccb9f

github-actions bot commented Jan 21, 2025

✅ Forge suite realistic_env_max_load success on 42f5a779eada739e117a9d4873e838afb98ccb9f

grao1991 commented Jan 21, 2025 •

edited

Loading

trunk-io bot commented Jan 21, 2025 •

edited

Loading

larry-aptos Jan 21, 2025 •

edited

Loading

✅ Forge suite `compat` success on `bb609ba7417025d8af1e199c345631c8b4b2c412` ==> `42f5a779eada739e117a9d4873e838afb98ccb9f`

✅ Forge suite `realistic_env_max_load` success on `42f5a779eada739e117a9d4873e838afb98ccb9f`