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
Convert DHT to libp2p backend #296
Convert DHT to libp2p backend #296
Changes from 70 commits
4750880
a822899
ebbfe30
ad17dbb
4431d86
bd398d1
75e6ac7
679be89
2ff6e2c
1104e23
64387cb
1041751
f2e49b3
ef92722
3fdb80c
a11b0d4
6bbe58e
2cf54be
759d0dc
ab38ce8
47aadba
72f4258
6f65bde
f73ca6c
6bacf52
7a3f8d7
5acd1dd
8fc19d4
f148e44
0a44f07
c4918f9
1da829e
8d72387
74fc538
7b17300
6a9e6fe
a42d111
382028e
5e03bec
5b3e015
7e7ac43
25f4c77
717b62f
07e4baf
f9298e3
518031c
3f35c2d
93ac771
475d520
b7b82cb
5cffa06
dec4f4c
48dad87
73c5369
231bcee
daeeba8
f93946a
80dbde8
4e8deb5
2cd8152
626569c
a7f57ce
342a07b
a788251
7706183
96a57fe
19b4833
d41a978
5f3afeb
a4b31b6
3c7b903
f60681e
7435541
11eaaf7
1d6cc81
4959ca0
1774afb
c16ad2a
c8d3863
6c0bc1a
4cff374
b87fbc9
1732a19
960431c
deaaf77
8538e4b
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.
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.
I would argue that the proposed message is more misleading (looks like the DNS tells us the IP address for some domain since that's what DNS usually does). However, I don't mind changing the message to another, more clear one.
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.
What about
"Received IP address of monitor from Google DNS: {address}"
?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.
I'd say
monitor
is an ambiguous term. I've changed it toReceived public IP address of this machine from Google DNS
.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.
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 is something that the user will rarely want to directly connect to, since we're mainly interested in DHT multiaddrs)
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.
While the DHT works with a set of multiaddrs for one peer, we need to select only one IP for the averager to announce here (until we convert the averager itself to libp2p, which is outside of scope of this PR).
By default, this code chooses a public IPv4 address (if available) among all IPs in the visible multiaddrs. However, I'm afraid that this is not always the right choice. If it is not, a user seeing this message will get a chance to spot that the address is wrong and set the announced averager endpoint manually via the
announced_host
parameter.In any case, this message will be removed soon when the averager itself will be converted to work over libp2p entirely (that is, with several multiaddrs for a peer).
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.
Got it, though I'm generally against logging too much info in case everything proceeds as normal. If the user does run into errors, isn't it a logical next step to switch to debug and see what goes wrong?
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.
As a potential middle-ground, we could replicate the behavior from hivemind.optim:
(i have no preferences here, let @borzunov decide)
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.
I would leave it to be
logging.info()
for a short transition period when the DHT already uses libp2p but the averager does not use it yet. After this period, this line will be removed.