Add a peer store to network actor #208
Conversation
src/fiber/graph.rs
Outdated
| assert_eq!(connected_peers.len(), 0); | ||
|
|
||
| network_graph.load_from_store(); | ||
| let connected_peers = network_graph.get_connected_peers(); |
There was a problem hiding this comment.
can we change this unit test for PersistentNetworkActorState.
just to make sure the apis work as expected will be good.
There was a problem hiding this comment.
I wanted to make a unit test for this. The problem now for me is that NetworkActorState is too complex. In order to create a NetworkActorState, we almost certainly need to create a NetworkActor. The pr #205 actually has an easier way to create and restart a network actor. I think that would make the test easier. Admittedly, that is kind of like an integration test. And we still need to solve the problem of how to easily write unit tests for NetworkActorState.
src/fiber/network.rs
Outdated
| .store | ||
| .get_network_actor_state(&my_peer_id) | ||
| .unwrap_or_default(); | ||
| let mut peers_to_connect = persistent_state.get_peers_to_connect(); |
There was a problem hiding this comment.
any special reason to put persistent_state in NetworkActorState but not in NetworkActor? since in other actors like the ChannelActor, we use a store for persistent storage and ChannelActorState don't have a field for persistent_state.
There was a problem hiding this comment.
This is because most fields in the network actor state are volatile, I add a persistent state to make the fact that these fields should be persisted more explicit.
There was a problem hiding this comment.
I feel the name PersistentNetworkActorState kind of sucks. It is actually the NetworkActorStateRequiringPersistence.
src/fiber/network.rs
Outdated
| @@ -3196,6 +3252,9 @@ where | |||
| if let Err(err) = state.control.close().await { | |||
| error!("Failed to close tentacle service: {}", err); | |||
| } | |||
| debug!("Saving network actor state for {:?}", state.peer_id); | |||
| self.store | |||
| .insert_network_actor_state(&state.peer_id, state.persistent_state.clone()); | |||
There was a problem hiding this comment.
persistent_state make it confusing since it actually only works like a memory storage, maybe we need to add a store field for persistent_state,
and update the changes into store, so that we can make sure data won't be lost event process is killed.
There was a problem hiding this comment.
If I am going to add a store to the persistent_state, then there will be a cyclic dependency chain. The definition of the trait NetworkActorStateStore requires a PersistentNetworkActorState because we have to define what we want to save to/load from the store (this is the struct PersistentNetworkActorState). And the definition of PersistentNetworkActorState contains the field NetworkActorStateStore, hence a dependency cycle. We should do something to ensure that we will not forget to save the state to persistent storage. This is the reason why I wrapped the function save_peer_addresses of PersistentNetworkActorState (which we should not call directly, we can only enforce this by rust's field/methods visibility) to a function save_peer_addresses of NetworkActorState (which does the final persistence work, and we should use this when possible).
contrun
force-pushed
the
network-actor-connected-peers
branch
2 times, most recently
from
0eb9ce4
to
1ce1dd7
Compare
contrun
force-pushed
the
network-actor-connected-peers
branch
2 times, most recently
from
960d4f4
to
9e21450
Compare
Save peer information to network actor state Maybe connect to peer when on node annoouncement received Persist state when peer addresses changed
contrun
force-pushed
the
network-actor-connected-peers
branch
from
9e21450
to
4033caa
Compare
Closes #207