bitswap: don't re-provide blocks we've provided very recently #3105
Conversation
|
@jbenet the fact that adding a file again makes a reprovide happen is currently a side effect, and isnt exactly documented expected behaviour. I don't believe that changing that, and adding a separate command to reprovide a hash would count as a 'breaking change' do you? |
It's not officially one, no. it's undocumented behavior. that said, it's THE main way that i get something to work often-- meaning many times adding again will cause provides to actually get to more nodes and help the gateway find me. I would worry that this solution would decrease significantly the UX, i guess making it clear to use the other command may be ok. In general we just need to improve content routing as a whole. |
|
We should quantify how much BW this is expected to improve on real workloads. |
|
In a few tests i ran, it made less than half the number of provide RPCs over the course of a workload modeled after the one @parkan described in that other issue. I can measure bandwidth soon. |
|
in a few tests i ran using iptb, in a cluster of 10 nodes, where one of them adds the go-ipfs source directory:
|
License: MIT Signed-off-by: Jeromy <[email protected]>
whyrusleeping
force-pushed
the
feat/bitswap/dup-prov-cache
branch
from
b6b64a9
to
582e5de
Compare
|
Ran more detailed tests, The process was a fresh repo each time, connecting to the main network and adding the ipfs source tree. Bandwidth utilization was recorded by nethogs, i ran the test two times per ipfs version (would have done more, but results were fairly stable and thats a lot of data)
|
|
@Kubuxu CR? |
|
LGTM. |
|
travis CI OSX tests appear to be failling for all our repos even though they don't report any actual failures. |
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var ks []key.Key | ||
| for _, b := range bs { | ||
| for _, b := range toput { | ||
| if err := s.Exchange.HasBlock(b); err != nil { |
There was a problem hiding this comment.
@whyrusleeping, this change modifies the return value so that only the keys that don't already exist are returned. Was this intentional?
Redo how pull request #3105 is implemented. Instead of calling Has() is the blockservice modify the blockstore interface to return the blocks actually added. License: MIT Signed-off-by: Kevin Atkinson <[email protected]>
ghost
mentioned this pull request
Lots of operations will store the same block multiple times in rapid succession, we have optimizations to avoid putting those to disk multiple times, but nothing to avoid providing them multiple times.
This is a solution. The other option is to use the same logic as the blockstore (if we have the block already, noop) for providing. The issue with that is that you would no longer be able to re-add a file as a way to re-provide it.
cc @jbenet @Kubuxu for feedback
License: MIT
Signed-off-by: Jeromy [email protected]