Skip to content
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

Replace dead peers & increase replacement cache size #59

Merged
merged 4 commits into from
Mar 20, 2020

Conversation

aarshkshah1992
Copy link
Contributor

@Stebalien @aschmahmann

For #58.

  1. When the DHT marks a peer as Dead(because of protocol changes etc), we immediately try to replace it.
  2. Rather than frequently iterating over connected peers to identify potential candidates, have simply increased the size of the replacement cache for now. This should work fine in practise.
  3. Removed use of the goprocess lib completely as just context is fine here.

@aarshkshah1992
Copy link
Contributor Author

ping @Stebalien @aschmahmann

table.go Show resolved Hide resolved
table.go Outdated

if !notEmpty {
log.Infof("failed to replace missing peer=%s as all candidates were invalid", p)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we doing anything with the replacement peer?

Copy link
Contributor Author

@aarshkshah1992 aarshkshah1992 Mar 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. peer to be replaced -> we use it to calculate the CPL. But, we can just pass in the CPL in the channel rather than the peer.
  2. The peer we replace with -> we just "validate it". The way things are wried right now, a side effect of the validation is that that the peer gets added to the RT because of the DHT connectedness notification handler. I've documented that in the cleanup function. Should I document it here as well ?

@aarshkshah1992
Copy link
Contributor Author

aarshkshah1992 commented Mar 17, 2020

There is a bigger issue here. If we are already connected to a candidate, we wont add it to the RT because the connected notification in the DHT will not fire for it.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the Info log statements here should be Debug. Other than that, LGTM with some nits.

table_cleanup.go Outdated
i := rt.bucketIdForPeer(pinfo.Id)
if rt.buckets[i].getPeer(pinfo.Id) != nil {
log.Infof("successfully validated missing peer=%s, marking it as active", pinfo.Id)
rt.addPeer(pinfo.Id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just set the peer state to active here instead of calling addPeer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}

// replaces a peer using a valid peer from the replacement cache
func (rt *RoutingTable) startPeerReplacement() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need another goroutine or can we just have a single "run" goroutine?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replacement is also requested by the DHT in addition to the cleanup routine. What do you have in mind when you say have a single "run" go-routine ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean merge the cleanup and replacement goroutines to reduce the number fo components updating the routing table. But it's not really an important change.

@aarshkshah1992
Copy link
Contributor Author

Have changed the logs to Debug.

@aarshkshah1992 aarshkshah1992 merged commit d1a1e92 into master Mar 20, 2020
@Stebalien Stebalien deleted the feat/better-replacement branch March 23, 2020 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants