satellite,storagenode,bootstrap: add contact service to peer #2951
Conversation
thepaul
added
Request Code Review
ghost
removed their request for review
| @@ -352,6 +357,11 @@ func New(log *zap.Logger, full *identity.FullIdentity, db DB, revocationDB exten | |||
| pb.RegisterKadInspectorServer(peer.Server.PrivateGRPC(), peer.Kademlia.Inspector) | |||
| } | |||
|
|
|||
| { // setup contact service | |||
| log.Debug("Setting up contact service") | |||
| peer.Contact.Service = contact.NewService(peer.Log.Named("contact"), peer.Overlay.Service, peer.Transport) | |||
There was a problem hiding this comment.
do we need an endpoint for the SA peer as well that has the contact.PingNode() rpc?
There was a problem hiding this comment.
I don't think so. I'm worried if we did that, that it would be confusing for PingNode to be serviced by contact.Service on the nodes and contact.Service on the satellites, but where those two contact.Service's are totally different things :/
There was a problem hiding this comment.
lol. im confused. ok well lets merge this and sort it out in my next ticket.
There was a problem hiding this comment.
Heh. I mean, cause, storagenode/contact.Service (or Endpoint) and satellite/contact.Service (or Endpoint) are totally different things
bootstrap/peer.go
Outdated
| @@ -76,6 +77,11 @@ type Peer struct { | |||
| Inspector *kademlia.Inspector | |||
| } | |||
|
|
|||
| Contact struct { | |||
There was a problem hiding this comment.
do we need a comment here?
There was a problem hiding this comment.
Not according to the linter, and not according to current practice (see the other nearby structs). But we could sure add one if it is needed for readability
There was a problem hiding this comment.
no need to add one, i just thought the linter would complain
| @@ -20,7 +20,7 @@ var mon = monkit.Package() | |||
| // Service is the contact service between storage nodes and satellites | |||
| type Service struct { | |||
| log *zap.Logger | |||
| self *overlay.NodeDossier | |||
| self overlay.NodeDossier | |||
There was a problem hiding this comment.
just out of curiosity why make this a value instead of a pointer?
There was a problem hiding this comment.
Solely for the sake of being able to pass the result of peer.Kademlia.RoutingTable.Local() into the NewService constructor. But also, it looks like that's how we generally pass these around. We don't need it to be a pointer (it doesn't need to be changed from elsewhere), and the overhead of copying the extra bytes for copy-by-value is probably outweighed by the overhead of gc stuff when we use a pointer.
There was a problem hiding this comment.
I noticed that too... I bet it's so this service doesn't accidentally overwrite any of the nodeDossier data
bootstrap doesn't have an overlay.
What:
Adds contact services and endpoints as necessary to satellite and storagenode.
Why:
We will need these once we remove kademlia.
Please describe the tests:
Please describe the performance impact:
None
Code Review Checklist (to be filled out by reviewer)