swarm/network/stream: use bzzAddr for proximity calculation#1360
swarm/network/stream: use bzzAddr for proximity calculation#1360holisticode wants to merge 2 commits intoswarm-rather-stable-update-syncingfrom
Conversation
| // Peer is the Peer extension for the streaming protocol | ||
| type Peer struct { | ||
| *protocols.Peer | ||
| bzzPeer *network.BzzPeer |
There was a problem hiding this comment.
The bzzPeer has access to the protocols.Peer, so maybe we don't need the embedding on line 60 ?
There was a problem hiding this comment.
I applied a change to address this which resulted in quite a lot of cascading changes...I hope this does not introduce some new level of "ugliness"?
There was a problem hiding this comment.
@holisticode all the peer structs are confusing to me. I suggested the change in case it didn't result in many cascading changes. If this is not the case, feel free to just address this particular problem, and once this is correct, we can review and discuss how to refactor :)
There was a problem hiding this comment.
@holisticode I am sorry if you have understood my comment as a request to do that change - it was not intended as such.
I think the new cascading change is much worse to what we have right now, and will also result in many conflicts with other branches we have in progress right now.
Furthermore this cascading change is not addressing the underlying problem - which is the mess of all the different peer structs and their inheritance.
I suggest we just fix the proximity calculations if possible, and not do such cascading change throughout the codebase, unless we actually address the underlying problem of the design.
| func NewPeer(peer *network.BzzPeer, streamer *Registry) *Peer { | ||
| p := &Peer{ | ||
| Peer: peer, | ||
| Peer: peer.Peer, |
There was a problem hiding this comment.
Explicitly keeping a pointer to peer.Peer seems redundant.
| var cancel func() | ||
| // TODO: do something with this hardcoded timeout, maybe use TTL in the future | ||
| ctx = context.WithValue(ctx, "peer", sp.ID().String()) | ||
| ctx = context.WithValue(ctx, "peer", sp.bzzPeer.ID().String()) |
There was a problem hiding this comment.
This is changing behaviour on master. One of the things that I am addressing in simple-fetchers is the use of context as a container for values that change the logic of execution.
I am not sure if it is okay to change enode.ID to bzzAddr here.
| // TODO: Enable this log line | ||
| // log.Warn("invalid chunk delivered", "peer", sp.ID(), "chunk", msg.Addr, ) | ||
| msg.peer.Drop() | ||
| msg.peer.bzzPeer.Drop() |
There was a problem hiding this comment.
Why do we have to change this? I think you are changing way too many places from peer to bzzPeer, which is essentially the same.
| } | ||
|
|
||
| log.Trace("handle.chunk.delivery", "ref", msg.Addr, "from peer", sp.ID()) | ||
| log.Trace("handle.chunk.delivery", "ref", msg.Addr, "from peer", sp.bzzPeer.ID()) |
There was a problem hiding this comment.
Why is this necessary now, if we are not going to be refactoring all the peers in this PR?
| // exchange between peers over p2p. Instead, error will be returned | ||
| // only if there is one from sending subscribe error message. | ||
| err = p.Send(ctx, SubscribeErrorMsg{ | ||
| err = p.bzzPeer.Send(ctx, SubscribeErrorMsg{ |
There was a problem hiding this comment.
Why do we need to change this as well? Whether we call Send on p or bzzPeer is the same, no?
|
Obsolete due to #1336 |
2 participants
This PR uses
BzzAddrinstead ofenode.IDto usechunk.Proximity()forpocalculations between nodes.This PR currently passes the tests but it is important to understand that it produces errors which do NOT result in the tests failing....check the logs!