api, metrics, network: check caps when deciding on next peer for a chunk #1749
Conversation
| @@ -69,7 +69,7 @@ func (i *Inspector) DeliveriesPerPeer() map[string]int64 { | |||
| // iterate connection in kademlia | |||
| i.hive.Kademlia.EachConn(nil, 255, func(p *network.Peer, po int) bool { | |||
| // get how many chunks we receive for retrieve requests per peer | |||
| peermetric := fmt.Sprintf("chunk.delivery.%x", p.Over()[:16]) | |||
| peermetric := fmt.Sprintf("network.retrieve.chunk.delivery.%x", p.Over()[:16]) | |||
There was a problem hiding this comment.
Independent of the findPeer - RequestFromPeers bug, but nonetheless a nice-to-have fix, so that DeliveriesFromPeers debug API works.
| @@ -121,7 +121,7 @@ func datadirDiskUsage(path string, d time.Duration) { | |||
| for range time.Tick(d) { | |||
| bytes, err := dirSize(path) | |||
| if err != nil { | |||
| log.Warn("cannot get disk space", "err", err) | |||
| log.Trace("cannot get disk space", "err", err) | |||
There was a problem hiding this comment.
This warning is annoying when we hit LevelDB during compaction, and it is mostly useless in our monitoring system, therefore I am reducing its level.
| @@ -231,6 +232,10 @@ func (r *Retrieval) findPeer(ctx context.Context, req *storage.Request) (retPeer | |||
| return true | |||
There was a problem hiding this comment.
With the p.HasCap change, I think we don't need this if p.LightNode check, @janos what do you think?
There was a problem hiding this comment.
Reverted it back, because we change the check for capabilities for bzz-retrieve, and not bzz-stream.
| @@ -231,6 +232,10 @@ func (r *Retrieval) findPeer(ctx context.Context, req *storage.Request) (retPeer | |||
| return true | |||
network/retrieval/retrieve.go
Outdated
| @@ -34,6 +34,7 @@ import ( | |||
| "github.com/ethereum/go-ethereum/rpc" | |||
| "github.com/ethersphere/swarm/chunk" | |||
| "github.com/ethersphere/swarm/network" | |||
| "github.com/ethersphere/swarm/network/stream/v2" | |||
There was a problem hiding this comment.
Actually, there is a circular dependency in tests reported by linter. Retrieval is needed in stream tests, and here stream is imported.
There was a problem hiding this comment.
Right, so I will just hard-code the name of the protocol. This is not something that should change often. OK?
nonsense
force-pushed
the
retrieve-bootnode-findpeer
branch
from
4d37c8c to
ecf0094
Compare
The
findPeerfunction, which decides which peer theretrieveprotocol should ask for a chunk, currently does not check if the returned peer supports thestreamprotocol - it only ignores light nodes. Because of that it is possible for it to return abootnodein a given network.So this PR is adding a filter in the
findPeerfunction, so that we only suggest peers that run thestreamprotocol. With that change we might not need to even check for light nodes (theif-condition above).