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

more discriminate connectivity #555

Merged
merged 2 commits into from Apr 15, 2015

Conversation

rade
Copy link
Member

@rade rade commented Apr 14, 2015

If a peer was specified on the command line w/o a port, then do not connect to it if we have inbound connections from that IP.

This eliminates unnecessary (and ultimately failing) connection attempts in the common scenario of several peers having been told about each other on their command lines.

Closes #478.

@rade rade assigned awh Apr 14, 2015
@awh
Copy link
Contributor

awh commented Apr 14, 2015

Looks good to me, although there is a typo in the commit message...

@rade rade assigned rade and unassigned awh Apr 14, 2015
@rade rade force-pushed the 478_more_discriminate_connectivity branch from a471c0f to f9d3b79 Compare April 14, 2015 12:21
@rade rade assigned awh and unassigned rade Apr 14, 2015
@rade
Copy link
Member Author

rade commented Apr 14, 2015

Fixed the typo. And committed a change with a different internal representation for command line supplied addresses. I think it's marginally cleaner and clearer. If you agree I'll happily squash those two commits into one.

The last commit is related tidying up in other parts of the code base as a consequence of the 2nd commit. Technically it could be split into two - removal of the NormalisePeerAddr call site and removal of NormalisePeerAddr. The former has no dependency on the other changes. The latter would depend on the lot.

@rade rade force-pushed the 478_more_discriminate_connectivity branch from f9d3b79 to ba760b4 Compare April 14, 2015 21:17
ourself *LocalPeer
peers *Peers
port int
targets map[string]*Target

This comment was marked as abuse.

This comment was marked as abuse.

@awh
Copy link
Contributor

awh commented Apr 15, 2015

Yep agree the use of net.TCPAddr is an improvement - squash as you see fit and I'll merge.

@rade rade assigned rade and unassigned awh Apr 15, 2015
If a peer was specified on the command line w/o a port, then do not
connect to it if we have inbound connections from that IP.

This eliminates unnecessary (and ultimately failing) connection
attempts in the common scenario of several peers having been told
about each other on their command lines.

We represent a command line peer address with a TCPAddr, which avoids
back and forth conversion to string and extra fields to carry the "did
the original include a port?" information.
There was only one call site left and it always gets invoked with an
ip:port, so normalisation is a no-op.

Also remove mysterious comment.
@rade rade force-pushed the 478_more_discriminate_connectivity branch from ba760b4 to 339f85c Compare April 15, 2015 10:45
rade added a commit that referenced this pull request Apr 15, 2015
@rade rade merged commit ab49eb4 into weaveworks:master Apr 15, 2015
@rade rade modified the milestone: 0.10.0 Apr 18, 2015
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.

Indefinite reconnects occur if two peers are launched referencing each other
2 participants