-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
optimize numInbound count #960
Conversation
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.
Generally LGTM. However, there are a few things that I'm not sure about and commented on that may just be due to my unfamiliarity with the intricacies of this code.
@@ -0,0 +1,35 @@ | |||
package identify |
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.
For posterity. why'd we move this to a new file/what is a reader supposed to think when distinguishing between obsaddr_test.go
and observed_addr_test.go
. It seems like these names are too similar.
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.
This needed access to private types so I moved it from the identify_test
package to the identify
package.
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.
seems reasonable. nbd, but maybe we should name the files differently?
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.
Probably.
// Don't trump an outbound observation with an inbound | ||
// one. |
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.
Do you mean the opposite? You're saying to mark as inbound if it was, or is currently inbound.
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.
Fixed, I think.
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.
I think the wording here is just throwing me off (maybe just tired 😃). Maybe something like "Observation is considered inbound if the peer ever gave it to us as an inbound observation"
Seems reasonable, although we need to figure out why CI is failing here |
We don't use and/or expose these anyways. Making them private makes it easier to reason about their state.
We prefer addresses from inbound connections. We don't want outbound connections to hide these perfectly good addresses.
We call this _very_ frequently when computing our local addresses.
Well, the comment above the failure is:
Ant it appears to be very rare. |
We call this very frequently when computing our local addresses.
This PR also:
Review commit by commit.