-
Notifications
You must be signed in to change notification settings - Fork 224
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
dht query improvements #159
Conversation
(this is a WIP as in, i'm still looking at other change to make to the query code. These changes by themselves are really ready to go) |
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. Just a cosmetic change & comment request.
@@ -38,7 +39,8 @@ type dhtQueryResult struct { | |||
closerPeers []*pstore.PeerInfo // * | |||
success bool | |||
|
|||
finalSet *pset.PeerSet | |||
finalSet *pset.PeerSet |
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.
go fmt
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.
hrm? This is go fmt'ed
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.
my bad!
query.go
Outdated
@@ -236,7 +241,7 @@ func (r *dhtQueryRunner) queryPeer(proc process.Process, p peer.ID) { | |||
|
|||
// make sure we're connected to the peer. | |||
// FIXME abstract away into the network layer | |||
if conns := r.query.dht.host.Network().ConnsToPeer(p); len(conns) == 0 { | |||
if r.query.dht.host.Network().Connectedness(p) == inet.NotConnected { |
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.
maybe add a comment to the effect of "failure to establish a connection will cause the function to short circuit in this block" to make the function flow read easier
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.
👌 done
9d83941
to
6b33279
Compare
This is a bit of a WIP, but these two changes should be pretty good on their own.
The first is relatively minor, and just uses a more optimal method for checking if we have a connection to a peer.
The second one changes
GetClosestPeers
to only return the peers from its query that it was actually able to connect to. This is strictly better than the current code, as currently we return a set of peers that we are either already connected to (and the query knows this) or we have already failed to dial them (and the query knows this). Where the end result is we basically fail to put records to the peers that the query has already failed to connect to. So theres really no point in returning these peers to the caller, as we just tried and failed to connect to them.