-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add ability to provide DNS seeds #19
Conversation
b858e88
to
80bc15e
Compare
It will be reused in a later commit.
5699fee
to
29da5b3
Compare
Rebased against v0.x.x. |
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.
LGTM, except one minor style fix
26df439
to
339f8ca
Compare
@Geod24 Fixed and squashed. |
Btw, I wasn't sure whether we can provide a list of initial nodes and also DNS seeds in the same config |
You didn't modify the call site tho |
source/agora/node/Network.d
Outdated
} | ||
|
||
/// try to discover the network until we found | ||
/// all the validator nodes from our quorum set. | ||
public void discover () | ||
{ | ||
logInfo("Resolving DNS from %s", this.dns_seeds); | ||
this.addAddresses(resolveDnsSeeds(this.dns_seeds)); |
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.
Also what that in a if (this.dns_seeds.length)
We don't want to make DNS seed mandatory for the moment, do we ?
It's currently part of a separate config field to cleanly separate between Agora nodes and DNS seeds. It may later be merged into a common section if necessary.
Rebased. |
I'm not sure we can test this in the repo until we implement integration testing. However a few manual test-runs did work with a DNS seed node.