Sdk 142 - Dns client #1
Conversation
…e peers Signed-off-by: Paolo Galli <[email protected]>
Signed-off-by: Paolo Galli <[email protected]>
Signed-off-by: Paolo Galli <[email protected]>
…internal peer database Signed-off-by: Paolo Galli <[email protected]>
Signed-off-by: Paolo Galli <[email protected]>
Sdk 143 - DnsClient lookup flow implementation
|
|
||
| val (ipv4Addresses, ipv6Addresses) = seeders.foldLeft(Seq[InetAddress]()) { (acc, curr) => | ||
| val newElements = if (acc.size < nodesThreshold) { | ||
| lookupFunction(curr) |
There was a problem hiding this comment.
I think should be better to handle excpetion during lookup (to proceed with the next seeder)
| case class DnsClientInput(dnsSeeders: Seq[DnsSeederDomain], lookupFunction: DnsSeederDomain => Seq[InetAddress] = defaultLookupFunction) | ||
|
|
||
| object DnsClientInput { | ||
| def defaultLookupFunction(url: DnsSeederDomain): Seq[InetAddress] = { |
There was a problem hiding this comment.
change return type to Try[Seq[InetAddress]], to allow exception handling in the caller
| @@ -91,6 +98,8 @@ class PeerManager(settings: ScorexSettings, scorexContext: ScorexContext) extend | |||
| peerSpec.declaredAddress.exists(isSelf) || peerSpec.localAddressOpt.exists(isSelf) | |||
| } | |||
|
|
|||
| private def peerDatabaseHasOnlyKnownPeersFromConfig(): Boolean = peerDatabase.knownPeers.size <= settings.network.knownPeers.size | |||
|
|
|||
There was a problem hiding this comment.
we should compare also the single element's value, not only the size
There was a problem hiding this comment.
We're comparing the size because it's considering a new feature we'll add. We're going to keep in the peer database all the addresses of the knownPeers, instead of deleting them after the first time we cannot connect. So, it's implicit that, at most, the internal peer database will contain a number of peers equal to the number of known peers
There was a problem hiding this comment.
ok got it, but let's add a comment
| @@ -23,7 +23,11 @@ class PeerManager(settings: ScorexSettings, scorexContext: ScorexContext) extend | |||
|
|
|||
| if (peerDatabase.isEmpty) { | |||
| // fill database with peers from config file if empty | |||
| settings.network.knownPeers.foreach { address => | |||
| fillPeerDatabase(settings.network.knownPeers) | |||
| } | |||
There was a problem hiding this comment.
we should also fire a GetNewPeers here , to first start the process at bootstrap
| @@ -65,6 +69,9 @@ class PeerManager(settings: ScorexSettings, scorexContext: ScorexContext) extend | |||
| case RemovePeer(address) => | |||
| log.info(s"$address removed from peers database") | |||
| peerDatabase.remove(address) | |||
| if (peerDatabaseHasOnlyKnownPeersFromConfig()) { | |||
| sender() ! EmptyPeerDatabase | |||
There was a problem hiding this comment.
is really needed to fire this EmptyPeerDatabase message?
Maybe is enough to fire direclty GetNewPeers, unless there is a reason to go always through the NetworkController
also: instead of firing the update when the new peers is empty maybe is worth to "ping" the seeders every a reasonable amount of time (like 30 minutes?). in this way we don't have to restart a node if the list returned by any of the seeds is changed. Is it possible to set Akka to fire a message every x minutes?
There was a problem hiding this comment.
This sounds like one of the options that we took into consideration: if we don't receive a new block in 15 minutes, then we need new peers because we're not connected to any forger node. The solution to check the internal database is simpler and we don't have to schedule any jobs.
Regarding passing through the NetworkController, it's more a pattern that is implementing scorex. So the peer manager, which is just a storage, detects that we don't have any more peer and tells it to the network controller, who's responsible to coordinate the different actors.
There was a problem hiding this comment.
after today discussion let'ls discard my comment (in reality the new peer will be broadcasted to the others anyway when it first tries to connect to another one).
Just one final note: I would change the message name from EmptyPeerDatabase to PeerDatabaseIsEmpty (more clear it notifies a state)
Signed-off-by: Paolo Galli <[email protected]>
…st intantiated + test Signed-off-by: Paolo Galli <[email protected]>
| lookupFunction(curr) match { | ||
| case Success(value) => value | ||
| case Failure(exception) => | ||
| exceptionMessages ++ exception.getMessage |
There was a problem hiding this comment.
log the exception also please
Signed-off-by: Paolo Galli <[email protected]>
|
@i-skrypnyk take a look to the PR later. Seems it touches the files/methods that are going to be changed by you in the other task. |
|
@paologalligit @i-Alex are we still going to include this one, or it can be closed? |
This won't be included in RC-10 release |
Create a new actor responsible for DNS seeder lookups (DnsClient) and the strategies it will be collecting IP addresses with: