Implement pending join pool #1587
Conversation
| (NodeJoinsTheCluster.getClass, 209, CompatibleSerializer), | ||
| (NodeLeavesTheCluster.getClass, 210, CompatibleSerializer), | ||
| (NodeKickedOutByHealthcheck.getClass, 211, CompatibleSerializer), | ||
| (classOf[NextActiveNodes], 212, CompatibleSerializer), |
There was a problem hiding this comment.
why not classOf[] as it was used before?
There was a problem hiding this comment.
Can you elaborate?
There was a problem hiding this comment.
I mean, you used Foo.getClass instead of classOf[Foo] as it was used almost everywhere else. that's why I'm asking ;)
There was a problem hiding this comment.
Ok you mean e.g. the line 121. That's how we got the class for the case objects. classOf[] doesn't work for case objects AFAIR
| lastSnapshot: String, | ||
| checkpointBlocks: Seq[String], | ||
| publicReputation: SortedMap[Id, Double], | ||
| nextActiveNodes: NextActiveNodes, |
There was a problem hiding this comment.
what if we will make a rollback and there are no such nodes in the cluster?
There was a problem hiding this comment.
Rollback case may not be handled yet. Essentially during rollback the NextActiveNodes should be ignored and we should start from the initial nodes - currently based on the whitelisting file. I mentioned it on the daily sometime ago.
There was a problem hiding this comment.
I guess it will be then NextActiveNodes(Set.empty, Set.empty) but @TheMMaciek please confirm
There was a problem hiding this comment.
During rollback these will be set to some nodes, but we need to ignore them during rollback and use the initialFullNodes -> currently these are first 3 nodes in the whitelisting file. The initial nodes logic can be changed of course, especially if we will remove whitelisting.
| @@ -51,7 +51,7 @@ class ConsensusManager[F[_]: Concurrent: ContextShift: Timer]( | |||
| nodeId: Id, | |||
| keyPair: KeyPair, | |||
| checkpointsQueueInstance: DataResolverCheckpointsEnqueue[F] | |||
| ) { | |||
| )(implicit F: Concurrent[F], CS: ContextShift[F], T: Timer[F]) { | |||
There was a problem hiding this comment.
I guess we wanted to have it uniform, so I moved all of them to this notation. I can move only what I need.
| def getActivePeersIds(withSelfId: Boolean = false): F[Set[Id]] | ||
| def getActivePeers: F[Map[Id, PeerData]] | ||
| def getActiveLightPeersIds(withSelfId: Boolean = false): F[Set[Id]] | ||
| def getActiveLightPeers(): F[Map[Id, PeerData]] |
There was a problem hiding this comment.
getActiveLightPeers instead of getActiveLightPeers() to be consistent with the other methods. if it makes an action, then (). if not and it is only a get, no ()
There was a problem hiding this comment.
That's exactly what happens, getActiveLightPeers() performs an action.
There was a problem hiding this comment.
You can look it up, but essentially what it does is it takes the list of lightPeersIds, and fetches their PeerData from the list of all peers. There is some more logic happening for this case and I'm not sure it should stay their inside a storage, but that's how I rebased it initially to test if the implementation still works. The thing is this function is needed in different places around the code base, it was previously in Cluster class, now it's in storage and it kind of clashes with the idea that storage should only store data so possibly I will need to improve it.
There was a problem hiding this comment.
but all in all, it's get active light peers. doesn't matter how it works under the hood. if it takes it from the database, API or Mars ;)
There was a problem hiding this comment.
Yeah kind of makes sense ;). I remember we discussed that only for a simple getter it should be parameterless (just getting the value) but here much more happens actually. Though it's true that behind the interface it's not distinguishable. That said I can change it to parameterless and we can assume that every get function with no parameters should be in parameterless notation, or we could drop that convention and use empty parameter list simply everywhere.
| def getActiveFullPeersIds(withSelfId: Boolean = false): F[Set[Id]] | ||
| def getActiveFullPeers(): F[Map[Id, PeerData]] | ||
| def setActiveFullPeers(peers: Set[Id]): F[Unit] | ||
| def isAnActivePeer: F[Boolean] |
There was a problem hiding this comment.
There is node and cluster. Node storage is about self-knowledge. Cluster storage about the other peers.
Here, isAnActivePeer takes no parameter so I think it should belong to node storage, not cluster storage.
There was a problem hiding this comment.
Sure, it still can be moved to where it fits more.
| // TODO: notify new light nodes to join the pool also and update your light nodes list | ||
| // TODO: we need a consensus over new light and full nodes between current active full nodes | ||
| private def notifyNewActivePeersAndLeaveThePool(): F[Unit] = | ||
| for { | ||
| _ <- logger.debug(s"Notifying new active peers to join the active pool!") |
There was a problem hiding this comment.
Why this is a part of redownload service? It should be separated
There was a problem hiding this comment.
I guess it can be moved. Redownload flow seemed like the best place to put it initially.
There was a problem hiding this comment.
imo it should be in a separate service. and the same for storage related to joining pool
|
|
||
| private val peers: Ref[F, Map[Id, PeerData]] = Ref.unsafe(Map.empty[Id, PeerData]) | ||
|
|
||
| // TODO: move initialization of the refs below to somewhere outside - Cluster class startup? |
There was a problem hiding this comment.
It's in startup flow currently. It was initialized during Cluster class startup till refs where moved to storage classes and because clusterStorage and nodeStorage are only aware of themselves the initial state (like initial full nodes set based on whitelisting file) can't be set during storage initial creation. I did put keyPair or nodeId in some storages but I'm not convinced it should be like that, so I still plan to move these outside of storage and handle logic that needs these parameters differently.
TheMMaciek
force-pushed
the
implement-pending-join-pool-REBASED
branch
from
40f825b
to
68b663d
Compare
No description provided.