Better RT refresh & mantainence #465
Conversation
aarshkshah1992
changed the title
|
ping @Stebalien |
There was a problem hiding this comment.
Mostly LGTM, nothing critical/blocking.
@aschmahmann could I get a second review on this? It'll probably need to be rebased.
| // startSelfLookup starts a go-routine that listens for requests to trigger a self walk on a dedicated channel | ||
| // and then sends the error status back on the error channel sent along with the request. | ||
| // if multiple callers "simultaneously" ask for a self walk, it performs ONLY one self walk and sends the same error status to all of them. | ||
| func (dht *IpfsDHT) startSelfLookup() error { |
There was a problem hiding this comment.
Revisiting this, I'm not sure if we need something quite this complicated. We can probably get away with just calling selfWalk from within both the refresh function and the "address changed" loop.
(although we'll need to do it asynchronously inside the "address changed" loop)
Note: this code looks fine. I'm just not sure if the complexity is warranted.
| @@ -17,7 +19,7 @@ var DefaultBootstrapPeers []multiaddr.Multiaddr | |||
|
|
|||
| // Minimum number of peers in the routing table. If we drop below this and we | |||
| // see a new peer, we trigger a bootstrap round. | |||
| var minRTRefreshThreshold = 4 | |||
| var minRTRefreshThreshold = 20 | |||
There was a problem hiding this comment.
this seems like a high minimum for triggering new bootstraps. was this for testing, or intentional?
There was a problem hiding this comment.
This is intentional. The current interval for a refresh is too much (1 hour) & we want to be more proactive in doing refreshes when needed rather than doing them to often without needing to do them. But really, we can always tune these params if we run into problems later.
| @@ -710,9 +750,9 @@ func (dht *IpfsDHT) handleProtocolChanges(proc goprocess.Process) { | |||
| // TODO: discuss how to handle this case | |||
There was a problem hiding this comment.
should this also trigger an update?
There was a problem hiding this comment.
@willscott Great spot ! It will after this code in Adin's PR goes in.
dht.go
Outdated
| evts := []interface{}{&event.EvtPeerIdentificationCompleted{}, &event.EvtPeerIdentificationFailed{}} | ||
| dht.subscriptions.evtPeerIdentification, err = h.EventBus().Subscribe(evts, eventbus.BufSize(256)) | ||
| if err != nil { | ||
| logger.Errorf("dht not subscribed to peer identification events; things will fail; err: %s", err) | ||
| } | ||
|
|
||
| dht.subscriptions.everPeerAddressChanged, err = h.EventBus().Subscribe(&event.EvtLocalAddressesUpdated{}, eventbus.BufSize(256)) | ||
| if err != nil { | ||
| logger.Errorf("dht not sybscribed to peer address changed event; err=%s ", err) |
There was a problem hiding this comment.
| logger.Errorf("dht not sybscribed to peer address changed event; err=%s ", err) | |
| logger.Errorf("dht not subscribed to peer address changed event; err=%s ", err) |
There was a problem hiding this comment.
@willscott Oh that's just how we spell it down here. It's a very us thing.
dht_bootstrap.go
Outdated
| } | ||
|
|
||
| // Batch multiple refresh requests if they're all waiting at the same time. | ||
| collectWaiting: |
There was a problem hiding this comment.
can this loop of collecting all waiting channels be broken out into a separate function?
There was a problem hiding this comment.
This is done.
dht_bootstrap.go
Outdated
| return | ||
| } | ||
| switch evt.(type) { | ||
| case event.EvtLocalAddressesUpdated: |
There was a problem hiding this comment.
since we only care about one type here, can we do this as
if _, ok := evt.(event.EvtLocalAddressesUpdated); ok { ... }
There was a problem hiding this comment.
This is done.
|
Status Update @Stebalien @aschmahmann @willscott I have addressed review comments that I've gotten till now. Will rebase this on the cypress branch once Adin's work on switching between client/server goes in and ask you guys to take one final look. |
|
Closing in favour of #482. |
@Stebalien
For #283, #434 & libp2p/go-libp2p#779.
Corresponding Routing Table PR:
libp2p/go-libp2p-kbucket#54