[indexer-alt] - add rpc api ingestion to indexer-alt #20787

patrickkuo · 2025-01-06T16:18:23Z

Description

add rpc api ingestion to indexer-alt, this is to unblock a private-net app indexer.

Test plan

How did you test the new or updated feature?

Release notes

Check each box that your changes affect. If none of the boxes relate to your changes, release notes aren't required.

For each box you select, include information after the relevant heading that describes the impact of your changes that a user might notice and any actions they must take to implement updates.

vercel · 2025-01-06T16:18:44Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 11, 2025 0:13am

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Feb 11, 2025 0:13am
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Feb 11, 2025 0:13am
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Feb 11, 2025 0:13am

bmwill · 2025-01-07T14:39:17Z

crates/sui-indexer-alt-framework/src/ingestion/mod.rs

+    #[clap(long)]
+    pub basic_auth: Option<String>,


Can you include some documentation of the expected format of this CLI arg, below you can see that its expected to be <username>:<password>.

+1 on the docs here, and some other comments:

Let's also clarify that it is only relevant if --rpc-api-url is set.

Consider accepting this value via an environment variable as well.

bmwill · 2025-01-07T14:41:22Z

crates/sui-rpc-api/src/client/mod.rs

+        let value: MetadataValue<Ascii> = format!("Basic {}", auth)
+            .parse()
+            .map_err(Into::into)
+            .map_err(Status::from_error)?;


Given this is a username/password lets make sure to set the sensitive flag on the MetadataValue:

https://docs.rs/tonic/latest/tonic/metadata/struct.MetadataValue.html#method.set_sensitive

bmwill · 2025-01-07T14:42:40Z

crates/sui-rpc-api/src/client/mod.rs

+                .tls_config(
+                    ClientTlsConfig::new()
+                        .with_enabled_roots()
+                        .assume_http2(true),


Is this needed? any TLS negotiation should include h2 in ALPN

this is needed for the full node we setup in the private net

amnn

Some high level thoughts but the change on the indexing framework side looks good here, I see that @bmwill has some unresolved comments on the RPC API side, so I will leave it with him for the final stamp.

amnn · 2025-01-13T16:10:56Z

crates/sui-indexer-alt-framework/src/ingestion/rpc_client.rs

+        Ok(Bytes::from(
+            Blob::encode(&data, BlobEncoding::Bcs)?.to_bytes(),
+        ))


I'm fairly sure we're going to decode this straight away, which is a little unfortunate -- is it worth changing the IngestionClientTrait so that fetch returns a CheckpointData?

Added FetchData, which can be either raw bytes or CheckpointData

amnn · 2025-01-13T16:12:01Z

crates/sui-indexer-alt-framework/src/ingestion/rpc_client.rs

+use sui_storage::blob::{Blob, BlobEncoding};
+use url::Url;
+
+pub(crate) struct RpcIngestionClient {


I'm in two minds about this myself, but do we need this type? (As opposed to implementing IngestionClientTrait directly on sui_rpc_api::Client?

crates/sui-indexer-alt-framework/src/ingestion/mod.rs

amnn · 2025-01-13T16:14:37Z

crates/sui-indexer-alt-framework/src/ingestion/mod.rs

+    #[clap(long)]
+    pub basic_auth: Option<String>,


+1 on the docs here, and some other comments:

Let's also clarify that it is only relevant if --rpc-api-url is set.

Consider accepting this value via an environment variable as well.

amnn · 2025-01-13T16:15:15Z

crates/sui-indexer-alt-framework/src/ingestion/mod.rs

+            let basic_auth = args.basic_auth.map(|s| {
+                let split = s.split(":").collect::<Vec<_>>();
+                assert_eq!(2, split.len());
+                (split[0].to_string(), split[1].to_string())
+            });


Why not accept this as two different flags to avoid hand-rolling the parser?

I am now using the username and password encoded in the URL instead of having separated args

amnn · 2025-01-13T16:15:45Z

crates/sui-indexer-alt-framework/src/ingestion/client.rs

@@ -76,6 +77,17 @@ impl IngestionClient {
        }
    }

+    pub(crate) fn new_rpc(


nit: move this above new_impl so it's with the other constructors?

bmwill · 2025-01-27T17:33:11Z

Note #20986 added support for configuring auth

patrickkuo · 2025-01-30T14:49:47Z

Note #20986 added support for configuring auth

thanks! that's great! I will rebase and update this PR

patrickkuo · 2025-02-03T16:41:40Z

crates/sui-rpc-api/src/client/mod.rs

@@ -150,7 +154,7 @@ impl Client {

        let (metadata, response, _extentions) = self
            .raw_client()
-            .max_decoding_message_size(64 * 1024 * 1024)
+            .max_decoding_message_size(128 * 1024 * 1024)


One of the checkpoint in our test environment exceeded 64Mb, I bumped the max decoding message size to avoid the error, @bmwill do you know what is the correct value to set here?

patrickkuo · 2025-02-03T16:42:11Z

this PR is ready for more review, thanks!

amnn · 2025-02-10T15:23:33Z

crates/sui-indexer-alt-framework/src/ingestion/rpc_client.rs

+impl IngestionClientTrait for RpcClient {
+    async fn fetch(&self, checkpoint: u64) -> FetchResult {
+        let data = self.get_full_checkpoint(checkpoint).await.map_err(|e| {
+            if e.message().contains("not found") {


Is there a more structured way to identify that a checkpoint is not found based on the response @bmwill ?

The errors are a bit of a mess still and i'm actively working on this as we speak. right now i think the gRPC Code::Unknown is always returned (which is not great)

There is a Code::NOT_FOUND though which will eventually be returned here (hopefully by the next release)

ok #21163 should address this and allow you to look at the error code directly

crates/sui-indexer-alt-framework/src/ingestion/rpc_client.rs

crates/sui-indexer-alt-framework/src/ingestion/mod.rs

amnn · 2025-02-10T15:58:47Z

crates/sui-indexer-alt-framework/src/ingestion/client.rs

+                    FetchData::CheckPointData(data) => {
+                        self.metrics
+                            .total_ingested_bytes
+                            .inc_by(size_of_val(&data) as u64);


size_of_val is not going to do the right thing here, because it's not going to look through pointers etc. @bmwill, is there a way to get similar information out from the gRPC client? If not, I would opt for not trying to replicate this metric in the case of RPC clients at all (but then we should update its documentation to mention that).

Not presently. but we could hook in something if we needed. Agreed that the way this is being done right now would produce inaccurate results.

I will remove this for RPC client for now until we have a better way to get the size information.

…via ALPN.

…eserilised CheckpointData * use username and password in URL instead of separated field * implement IngestionClientTrait for sui_rpc_api::Client instead of RpcIngestionClient

patrickkuo temporarily deployed to sui-typescript-aws-kms-test-env January 6, 2025 16:18 — with GitHub Actions Inactive

patrickkuo temporarily deployed to sui-typescript-aws-kms-test-env January 7, 2025 10:03 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 7, 2025 10:05 View deployment

patrickkuo temporarily deployed to sui-typescript-aws-kms-test-env January 7, 2025 11:00 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 7, 2025 11:01 View deployment

patrickkuo force-pushed the pat/indexer-rpc-api branch from 8b65939 to 5d6f78d Compare January 7, 2025 11:28

patrickkuo temporarily deployed to sui-typescript-aws-kms-test-env January 7, 2025 11:28 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 7, 2025 11:32 View deployment

patrickkuo force-pushed the pat/indexer-rpc-api branch from 5d6f78d to bf7ed0c Compare January 7, 2025 11:45

patrickkuo temporarily deployed to sui-typescript-aws-kms-test-env January 7, 2025 11:45 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 7, 2025 11:47 View deployment

patrickkuo temporarily deployed to sui-typescript-aws-kms-test-env January 7, 2025 12:00 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 7, 2025 12:02 View deployment

patrickkuo temporarily deployed to sui-typescript-aws-kms-test-env January 7, 2025 12:04 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 7, 2025 12:05 View deployment

patrickkuo temporarily deployed to sui-typescript-aws-kms-test-env January 7, 2025 12:40 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs January 7, 2025 12:41 View deployment

patrickkuo marked this pull request as ready for review January 7, 2025 14:01

patrickkuo temporarily deployed to sui-typescript-aws-kms-test-env January 7, 2025 14:01 — with GitHub Actions Inactive

patrickkuo requested review from bmwill, amnn and wlmyng January 7, 2025 14:01

bmwill reviewed Jan 7, 2025

View reviewed changes

amnn reviewed Jan 13, 2025

View reviewed changes

vercel bot deployed to Preview – sui-docs January 29, 2025 17:36 View deployment

patrickkuo force-pushed the pat/indexer-rpc-api branch from 18cd4de to 82b8fe2 Compare February 3, 2025 12:18

patrickkuo temporarily deployed to sui-typescript-aws-kms-test-env February 3, 2025 12:18 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 3, 2025 12:23 View deployment

patrickkuo temporarily deployed to sui-typescript-aws-kms-test-env February 3, 2025 16:11 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 3, 2025 16:13 View deployment

patrickkuo temporarily deployed to sui-typescript-aws-kms-test-env February 3, 2025 16:26 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 3, 2025 16:28 View deployment

patrickkuo commented Feb 3, 2025

View reviewed changes

patrickkuo requested review from bmwill and amnn February 3, 2025 16:42

patrickkuo force-pushed the pat/indexer-rpc-api branch from 542562a to e35b491 Compare February 10, 2025 15:00

patrickkuo temporarily deployed to sui-typescript-aws-kms-test-env February 10, 2025 15:00 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 10, 2025 15:05 View deployment

amnn reviewed Feb 10, 2025

View reviewed changes

patrickkuo added 10 commits February 11, 2025 11:26

add rpc api ingestion to indexer-alt

23228c6

group required for client args

37c101b

set assume_http2 true to support server without protocol negotiation …

19ffd6d

…via ALPN.

update base64 dep

ec37e37

fix test

b74556b

increase protobuf encoding size limit

22f7a04

fixup after rebase

2290cc5

* return FetchData instead of Bytes, Fetch data can be raw bytes or d…

f666621

…eserilised CheckpointData * use username and password in URL instead of separated field * implement IngestionClientTrait for sui_rpc_api::Client instead of RpcIngestionClient

minor fix

9401ada

address PR comment

1a47c37

patrickkuo force-pushed the pat/indexer-rpc-api branch from e35b491 to 1a47c37 Compare February 11, 2025 12:11

patrickkuo temporarily deployed to sui-typescript-aws-kms-test-env February 11, 2025 12:11 — with GitHub Actions Inactive

vercel bot deployed to Preview – sui-docs February 11, 2025 12:13 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[indexer-alt] - add rpc api ingestion to indexer-alt #20787

[indexer-alt] - add rpc api ingestion to indexer-alt #20787

patrickkuo commented Jan 6, 2025

vercel bot commented Jan 6, 2025 •

edited

Loading

bmwill Jan 7, 2025

amnn Jan 13, 2025

bmwill Jan 7, 2025

bmwill Jan 7, 2025

patrickkuo Feb 3, 2025

amnn left a comment

amnn Jan 13, 2025

patrickkuo Feb 3, 2025

amnn Jan 13, 2025

amnn Jan 13, 2025

amnn Jan 13, 2025

patrickkuo Feb 3, 2025

amnn Jan 13, 2025

bmwill commented Jan 27, 2025

patrickkuo commented Jan 30, 2025 •

edited

Loading

patrickkuo Feb 3, 2025

patrickkuo commented Feb 3, 2025

amnn Feb 10, 2025

bmwill Feb 10, 2025

bmwill Feb 10, 2025

bmwill Feb 10, 2025

amnn Feb 10, 2025

bmwill Feb 10, 2025

patrickkuo Feb 11, 2025

[indexer-alt] - add rpc api ingestion to indexer-alt #20787

Are you sure you want to change the base?

[indexer-alt] - add rpc api ingestion to indexer-alt #20787

Conversation

patrickkuo commented Jan 6, 2025

Description

Test plan

Release notes

vercel bot commented Jan 6, 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

amnn 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

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

bmwill commented Jan 27, 2025

patrickkuo commented Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

patrickkuo commented Feb 3, 2025

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

vercel bot commented Jan 6, 2025 •

edited

Loading

patrickkuo commented Jan 30, 2025 •

edited

Loading