feat(state-sync) State sync actors #9669
Conversation
chain/client/src/sync/sync_actor.rs
Outdated
| use super::adapter::SyncMessageForClient; | ||
| use near_network::state_sync::SyncActorMessageForNetwork; | ||
| struct MessageSenders { | ||
| client_adapter: Sender<SyncMessageForClient>, |
There was a problem hiding this comment.
nit: feel free to leave as is, but these two fields could just go directly in struct SyncActor right? seems simpler to me that way but it's a style preference, do it as you like it
There was a problem hiding this comment.
I would keep them bundled.
chain/client/src/sync/sync_actor.rs
Outdated
| use tracing::log::info; | ||
|
|
||
| use super::adapter::SyncMessageForClient; | ||
| use near_network::state_sync::SyncActorMessageForNetwork; |
There was a problem hiding this comment.
appears to be missing. Also, another nit on this and the SyncMessageForClient, but why not just call them something simpler like SyncMessage? We know it's meant for the network because it's in the near_network crate, and same for SyncMessageForClient, since it's defined by near_client
There was a problem hiding this comment.
I went for SyncMessage. My intention was to keep it clear where is this message intended to be used.
There was a problem hiding this comment.
where ... intended to be used
A good API is about what an object does and not about for what purpose it should be used.
There was a problem hiding this comment.
where ... intended to be used
A good API is about what an object does and not about for what purpose it should be used.
Sure, but I think the point still stands to be honest, because what it does is already covered by the name SyncMessage right? Like, I think it would be strange if we renamed near_network::types::NetworkRequests::Block to near_network::types::NetworkRequests::BlockForNetwork
There was a problem hiding this comment.
@marcelo-gonzalez I agree with you and my comment supported your point :)
VanBarbascu
force-pushed
the
state-sync-actors
branch
from
db3f46a
to
e4ff1b8
Compare
VanBarbascu
force-pushed
the
state-sync-actors
branch
2 times, most recently
from
68f230d
to
070d128
Compare
VanBarbascu
force-pushed
the
state-sync-actors
branch
from
070d128
to
5b101ba
Compare
VanBarbascu
force-pushed
the
state-sync-actors
branch
from
5b101ba
to
841588d
Compare
| where | ||
| M: Message + Send + 'static, | ||
| M::Result: Send, | ||
|
|
There was a problem hiding this comment.
The empty line seems unnecessary.
This is the first step in moving the state sync logic to model where one thread per shard will be started. Here we move the part application logic from `SyncJobsActor to the `SyncActor` added in near#9669. This means that the core of the logic is still in the client actor, but the requests to apply parts will be sent to different threads running these `SyncActor`s. In an ideal world, we'll be able to separate out more of the logic so that each of these actors can interact with the network layer, taking responsibility for downloading/requesting the parts themselves, but for now this PR will just take the first step towards that and set things up. Also we make a few changes to the scaffolding added in near#9669 to make things easier/more direct. * don't initialize the `SyncArbiter` in `nearcore::start_with_config()` like all the others, but instead just make it a field in the `ClientActor`, since its logic has no need to run separately in another thread or something. It's just managing the threads that the `ClientActor` knows how to start (how many, when to start them, etc). that means we don't need to store the client and network actors in that struct. * change `ShardUId` to `ShardId` throughout just to make things a little easier, since it shouldn't be incorrect. If there's a change in shard layout version, we can still just send requests to actors by shard ID
Create dummy state sync actors. Wrap them in an adapter that will be used to command and control them from client and network.
Create dummy state sync actors.
Wrap them in an adapter that will be used to command and control them from client and network.