refactor(indexer): Move `state_changes` from `StreamerMessage` root to `IndexerShard` instead #6255

khorolets · 2022-02-07T19:35:51Z

Extract indexer::streamer::types to separate crate indexer-primitives for publishing
Add GetStateChangesWithCauseInBlockForTrackedShards to client-primitives and corresponding view_client handler to return StateChangesWithCause for each shard separately
Breaking changes in StreamerMessage struct, there is no state_changes in the root anymore
Increase the streaming channel size from 16 to 100 (reindexing speed)

chain/client/src/view_client.rs

chain/indexer-primitives/Cargo.toml

chain/indexer/CHANGELOG.md

chain/indexer/Cargo.toml

chain/indexer/src/streamer/mod.rs

chain/indexer/src/streamer/utils.rs

chain/indexer-primitives/Cargo.toml

frol

Looks good to me.

P.S. Run fmt to fix CI

chain/indexer/CHANGELOG.md

chain/indexer/src/streamer/fetchers.rs

chain/client/src/view_client.rs

core/primitives/src/types.rs

matklad · 2022-02-08T11:23:25Z

chain/indexer-primitives/Cargo.toml

@@ -0,0 +1,19 @@
+[package]
+name = "near-indexer-primitives"


Could you add a readme explaining why this crates needs to exist, what should belong to this crate, and what shouldn't belong to this crate? "this crates holds types" doesn't answer those questions for me -- we might as well keep them in indexer, and we also might add some logic here, why not?

It seems to me that what actually happens is that "this crate defines over-the-wire schema for JSON messages used by indexer HTTP API". Ie, it seems that this is not about Rust, but rather about a network protocol/API we expose. That is, if true, a very important bit of information, and it currently is missing from the description.

🤔 wait, I think the above is not true. I've just tried removing Serialize, Deserialize from all intexer messages, and that seems to work -- nothing in this repository seems to rely on serialization. I guess, I am just confused as to why this particular bunch of types needs special treatment and is worth a separate crate :)

@matklad I'll add a little README.

We are making NEAR Lake - in a few words it's an indexer that is going to save all the data from the network to AWS S3 in JSON files. We are starting some NEAR Lake Framework which is going to read from AWS S3 and stream all the data. Both of the mentioned projects will use indexer primitives, in nearest future, there are going to be even more projects.

The first goal is to make it possible that projects like NEAR Lake Framework and the ones that use it as a dependency won't require an entire nearcore in dependencies in order to actually use only those primitives.

And we are going to extend the number of types in indexer primitives and adjust the serde_json parameters to improve the way the types are serialized.

I hope that makes sense. Let me know if you have questions regarding this extraction to a separate crate :)

Ah, this makes sense! And yeah, I want to stress that the salient point of this thing then is not the Rust types, but rather the JSON schema, and all of the related backwards compatibility concerns.

And yeah, this totally needs to be a separate crate. I wonder though if -types or -views would be a better public name than -primitives. In today's usage primitives is essentially a kitchen sink, and it defines much more than just data schema.

I also wonder if, long term, this should live in some other repository. It feels odd that this defines a data schema, but the actual data isn't used by the repo (so, it's easy for nearcore developers to break this stuff without realizing this). Contrast this with views, which also do define the schema, but this schema is than used by the code in this repository (the node's RPC API). It seems nicer if indexer-framework defines just the Rust API (without Serialize), and let's the consumers define their own data schema.

🤔 yeah, I guess long-term I can see two worlds:

In one world, "indexer schema" is something defined completely outside of nearcore as a part of the lake framework. In this world, there's a separate indexer-types crate, but it lives in the repo which implements the actual indexer. We can also imagine two different lakes by two different providers with different schema

In another world, we say that "indexer schema" is something universal, and is as much a part of nearcore as JSON RPC API. In this world, I would expect this to be a module in the crate that defines JSON RPC API (so, near_primitives::views::indexer today). In this world, while there might be alternative implementations of the lake, we'd expect them to use the same schema.

But yeah, this is long-term, starting with moving this to a separate crate might make sense (but also do consider just creating a views submodule)

@matklad TL;DR: There are several things at play, and you properly identified the long-term plans we actually have.

The types defined in this crate are Rust types for Indexer Framework, which is now tightly coupled with nearcore. You don't see any use of these APIs in nearcore repo because it is designed to be a library (you can find the usage in indexer-for-* projects out there), and we want to stabilize this API in the long term in terms of Rust types and while we already have our own top-level structure, there are still quite some types used in depth (e.g. Transaction and ExecutionOutcome types). Given there is an initiative to decouple JSON RPC types #5516, we decided to postpone solving all the problems.

Second, indeed, we want to be able to serialize the types for near-lake and deserialize it on the reader side without pulling the whole nearcore (we could have get away with pulling the whole nearcore, but due to #3803 it makes things a bit harder on our side, since we often receive questions about M1 support in our tooling and we use M1 ourselves)

Third, there is a plan to further solidify the APIs and extract near-indexer into a separate repo, but to make that happen, nearcore should be published as a crate, and its APIs also stable.

matklad · 2022-02-08T11:32:42Z

unrelated, but s/IndexerChunkView/IndexerChunk might make naming a bit more consistent.

mm-near · 2022-02-09T07:12:18Z

chain/indexer-primitives/src/lib.rs

@@ -8,7 +8,6 @@ pub use near_primitives::{types, views};
 pub struct StreamerMessage {
    pub block: views::BlockView,
    pub shards: Vec<IndexerShard>,


do you have to worry about backwards compatibility ? is this message used in communication between two different binaries ?

A little bit above in this PR we have talked about it, let me know if that doesn't answer your question

#6255 (comment)

mm-near · 2022-02-09T07:17:50Z

chain/indexer/src/streamer/mod.rs

+            receipt_execution_outcomes: vec![],
+            state_changes: state_changes
+                .remove(&shard_id)
+                .expect("StateChanges for given shard should be present"),


But some shards (chunks) might be missing in a block, right ?

Oh, really, my bad, thanks!

mm-near · 2022-02-09T07:18:28Z

chain/indexer-primitives/src/lib.rs

@@ -42,4 +41,5 @@ pub struct IndexerShard {
    pub shard_id: types::ShardId,
    pub chunk: Option<IndexerChunkView>,
    pub receipt_execution_outcomes: Vec<IndexerExecutionOutcomeWithReceipt>,
+    pub state_changes: views::StateChangesView,


do we have any tests for this ?

We had a conversation re: indexer and tests with @matklad recently

Here's the start #6195 (comment)

so - no tests ? :-(

bowenwang1996

This is fine as is, but I'd like to note the following:

We probably should change the underlying storage to store state changes by shard.
@frol @khorolets we plan to stop storing state changes in the database in the future Archival nodes do not need to store trie changes for old blocks #6250. It shouldn't affect how this PR works, but just want to give you a heads up.

khorolets · 2022-02-10T07:46:10Z

This is fine as is, but I'd like to note the following:

We probably should change the underlying storage to store state changes by shard.

@frol @khorolets we plan to stop storing state changes in the database in the future Archival nodes do not need to store trie changes for old blocks #6250. It shouldn't affect how this PR works, but just want to give you a heads up.

Thanks for the heads up @bowenwang1996! In #6250 you're saying it should be possible to retrieve StateChanges on the fly, so it means we would be able to return StateChanges from the NEAR Indexer side.

cc @frol

…o `IndexerShard` instead

Co-authored-by: Vlad Frolov <[email protected]>

mm-near · 2022-02-16T12:07:22Z

chain/indexer/src/lib.rs

-    pub fn streamer(&self) -> mpsc::Receiver<streamer::StreamerMessage> {
-        let (sender, receiver) = mpsc::channel(16);
+    pub fn streamer(&self) -> mpsc::Receiver<StreamerMessage> {
+        let (sender, receiver) = mpsc::channel(100);


why was it changed to 100? can you add a comment ?

In the PR description

Increase the streaming channel size from 16 to 100 (reindexing speed)

mm-near · 2022-02-16T12:09:40Z

chain/indexer-primitives/src/lib.rs

@@ -42,4 +41,5 @@ pub struct IndexerShard {
    pub shard_id: types::ShardId,
    pub chunk: Option<IndexerChunkView>,
    pub receipt_execution_outcomes: Vec<IndexerExecutionOutcomeWithReceipt>,
+    pub state_changes: views::StateChangesView,


so - no tests ? :-(

khorolets requested a review from frol February 7, 2022 19:35

khorolets requested review from bowenwang1996, mm-near, mzhangmzz and rtsai123 as code owners February 7, 2022 19:35

khorolets force-pushed the refactor/indexer-state-changes-by-shard-ids branch from f10e529 to 8c9d7d6 Compare February 7, 2022 19:36

frol requested changes Feb 7, 2022

View reviewed changes

miraclx approved these changes Feb 7, 2022

View reviewed changes

chain/indexer-primitives/Cargo.toml Outdated Show resolved Hide resolved

khorolets requested a review from matklad as a code owner February 8, 2022 09:09

khorolets force-pushed the refactor/indexer-state-changes-by-shard-ids branch from 9e694b2 to 4916e1b Compare February 8, 2022 09:09

frol approved these changes Feb 8, 2022

View reviewed changes

chain/indexer/CHANGELOG.md Outdated Show resolved Hide resolved

chain/indexer/CHANGELOG.md Outdated Show resolved Hide resolved

chain/indexer/src/streamer/fetchers.rs Outdated Show resolved Hide resolved

chain/client/src/view_client.rs Outdated Show resolved Hide resolved

matklad reviewed Feb 8, 2022

View reviewed changes

khorolets force-pushed the refactor/indexer-state-changes-by-shard-ids branch from 417e4a4 to f277032 Compare February 8, 2022 12:10

matklad approved these changes Feb 8, 2022

View reviewed changes

khorolets force-pushed the refactor/indexer-state-changes-by-shard-ids branch from 130ce16 to abf99e1 Compare February 8, 2022 16:25

khorolets mentioned this pull request Feb 8, 2022

Revise near_indexer::StreamerMessage struct near/near-lake-indexer#1

Closed

khorolets force-pushed the refactor/indexer-state-changes-by-shard-ids branch from abf99e1 to 204fec7 Compare February 8, 2022 18:18

mm-near suggested changes Feb 9, 2022

View reviewed changes

khorolets force-pushed the refactor/indexer-state-changes-by-shard-ids branch from 204fec7 to 8670763 Compare February 9, 2022 07:30

khorolets requested a review from mm-near February 9, 2022 07:30

bowenwang1996 reviewed Feb 10, 2022

View reviewed changes

khorolets and others added 7 commits February 15, 2022 13:17

refactor(indexer): Move state_changes from StreamerMessage root t…

eaa059f

…o `IndexerShard` instead

Add licence to indexer-primitives

43cb8fe

Address review suggestions

9cec349

Import order in Cargo.toml

5cd3b77

Update chain/indexer/CHANGELOG.md

2e39b31

Co-authored-by: Vlad Frolov <[email protected]>

Update chain/indexer/CHANGELOG.md

1834805

Co-authored-by: Vlad Frolov <[email protected]>

Address review suggestions. Add README to indexer-primitives crate

49b0652

khorolets added 3 commits February 15, 2022 13:17

Rename impl method, add @khorolets as codeowner of indexer-primitives

553937a

Fix to be ready that shard is missing when we retrieve state_changes

b8ad61f

Increase streaming channel from 16 to 100

5dbd4ac

khorolets force-pushed the refactor/indexer-state-changes-by-shard-ids branch from bd87154 to 5dbd4ac Compare February 15, 2022 11:17

mm-near approved these changes Feb 16, 2022

View reviewed changes

Merge branch 'master' into refactor/indexer-state-changes-by-shard-ids

7fe4bfc

khorolets added the S-automerge label Feb 16, 2022

near-bulldozer bot added 3 commits February 16, 2022 13:47

Merge refs/heads/master into refactor/indexer-state-changes-by-shard-ids

dc03906

Merge refs/heads/master into refactor/indexer-state-changes-by-shard-ids

b1db474

Merge refs/heads/master into refactor/indexer-state-changes-by-shard-ids

7e018d9

rtsai123 approved these changes Feb 16, 2022

View reviewed changes

near-bulldozer bot merged commit ea731e3 into master Feb 16, 2022

near-bulldozer bot deleted the refactor/indexer-state-changes-by-shard-ids branch February 16, 2022 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(indexer): Move `state_changes` from `StreamerMessage` root to `IndexerShard` instead #6255

refactor(indexer): Move `state_changes` from `StreamerMessage` root to `IndexerShard` instead #6255

khorolets commented Feb 7, 2022 •

edited

Loading

frol left a comment

matklad Feb 8, 2022

khorolets Feb 8, 2022

matklad Feb 8, 2022

frol Feb 8, 2022

matklad commented Feb 8, 2022

mm-near Feb 9, 2022

khorolets Feb 9, 2022

mm-near Feb 9, 2022

khorolets Feb 9, 2022

mm-near Feb 9, 2022

khorolets Feb 9, 2022

mm-near Feb 16, 2022

bowenwang1996 left a comment

khorolets commented Feb 10, 2022

mm-near Feb 16, 2022

khorolets Feb 16, 2022

mm-near Feb 16, 2022

refactor(indexer): Move state_changes from StreamerMessage root to IndexerShard instead #6255

refactor(indexer): Move state_changes from StreamerMessage root to IndexerShard instead #6255

Conversation

khorolets commented Feb 7, 2022 • edited Loading

frol 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

matklad commented Feb 8, 2022

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

bowenwang1996 left a comment

Choose a reason for hiding this comment

khorolets commented Feb 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refactor(indexer): Move `state_changes` from `StreamerMessage` root to `IndexerShard` instead #6255

refactor(indexer): Move `state_changes` from `StreamerMessage` root to `IndexerShard` instead #6255

khorolets commented Feb 7, 2022 •

edited

Loading