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

Add API endpoint to disconnect peers #23

Merged
merged 4 commits into from
Nov 16, 2018
Merged

Add API endpoint to disconnect peers #23

merged 4 commits into from
Nov 16, 2018

Conversation

vyzo
Copy link
Collaborator

@vyzo vyzo commented Nov 16, 2018

Closes #22

@ghost ghost assigned vyzo Nov 16, 2018
@ghost ghost added the in progress label Nov 16, 2018
@vyzo vyzo requested review from bigs and raulk November 16, 2018 11:35
connmgr.go Outdated
return errorResponse(err)
}

d.host.Network().ClosePeer(p)
Copy link
Member

Choose a reason for hiding this comment

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

ClosePeer returns an error. It's a synchronous call as it is, so we're going to wait until connections are closed anyway. Can we propagate the error to the caller?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, and we should.

connmgr.go Outdated
@@ -51,6 +51,15 @@ func (d *Daemon) doConnManager(req *pb.Request) *pb.Response {
d.host.ConnManager().TrimOpenConns(ctx)
return okResponse()

case pb.ConnManagerRequest_DISCONNECT_PEER:
Copy link
Member

Choose a reason for hiding this comment

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

Seems a bit of a misnomer to place this operation in the namespace of ConnManager? Why not put it in the same namespace as CONNECT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I will move it to top-level.

@vyzo
Copy link
Collaborator Author

vyzo commented Nov 16, 2018

Moved DISCONNECT to top-level (same as CONNECT) and also made sure to propagate the error.

Copy link
Member

@raulk raulk left a comment

Choose a reason for hiding this comment

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

LGTM. A few spacing nits but not important. Might be worth getting a review from @bigs before merge, as he's more involved in the daemon.

@vyzo vyzo merged commit 0542092 into master Nov 16, 2018
@ghost ghost removed the in progress label Nov 16, 2018
@vyzo vyzo deleted the feat/disconnect-peer branch November 16, 2018 18:10
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.

3 participants