Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sdk 142 - Dns client #1
base: dev
Are you sure you want to change the base?
Sdk 142 - Dns client #1
Changes from all commits
d86a457
8101192
29147d4
26d33fe
624c992
2678419
4546022
02e313a
4d95752
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should also fire a GetNewPeers here , to first start the process at bootstrap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should compare also the single element's value, not only the size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok got it, but let's add a comment