simple fetchers #1492
simple fetchers #1492
Conversation
… and Swarm connectivity is not a chain
nonsense
moved this from Backlog
to In review (includes Documentation)
in Swarm Core - Sprint planning
| return wait | ||
|
|
||
| return loaded, func(ctx context.Context) error { | ||
| select { |
There was a problem hiding this comment.
so should we not select on the ctx.Done?
There was a problem hiding this comment.
Why does this matter? Tests seem to be passing with the current select?
| if wait := c.NeedData(ctx, hash); wait != nil { | ||
| log.Trace("checking offered hash", "ref", fmt.Sprintf("%x", hash)) | ||
|
|
||
| if _, wait := c.NeedData(ctx, hash); wait != nil { |
There was a problem hiding this comment.
so we are still asking all offering peers, not just the first. note TODO here
There was a problem hiding this comment.
Yes, we are still asking all peers. We decided that we will be addressing this later, in order to reduce the scope of an already big PR.
| func (s *SwarmSyncerClient) NeedData(ctx context.Context, key []byte) (loaded bool, wait func(context.Context) error) { | ||
| start := time.Now() | ||
|
|
||
| fi, loaded, ok := s.netStore.GetOrCreateFetcher(ctx, key, "syncer") |
There was a problem hiding this comment.
TODO this logic should be part of netstore.Get no?
There was a problem hiding this comment.
I don't understand this question. Why do you want this part of Netstore.Get? In order to adhere to the interface of the syncer (which this PR is not addressing), we need to know how long to wait for a chunk, hence the returned Fetcher, and the select on <-fi.Delivered below.
If this is part of Netstore.Get, we are just pushing this complexity to the Netstore.Get, I don't really see why this is an improvement.
There was a problem hiding this comment.
| ) | ||
|
|
||
| // LNetStore is a wrapper of NetStore, which implements the chunk.Store interface. It is used only by the FileStore, | ||
| // the component used by the Swarm API to store and retrieve content and to split and join chunks. |
There was a problem hiding this comment.
TODO we should simplify this. AFAIU This extra object is only needed because Origin is not handled via peers to skip.
There was a problem hiding this comment.
How do you propose we simplify this?
This extra struct LNetStore is needed due to the FileStore, which has no notion of any network requests, or the NetStore, it just expects a simple Get interface to return chunks. We want to be able to test the FileStore independently of the NetStore.
Any concrete ideas are welcome.
Related discussion: #1344
This PR is a subset of the
simplify-fetchersbranch for easier review and mostly addressing the issues described at #1309It includes:
FetchFuncwith two new functions introduced are:You probably want to start reviewing this PR by checking them out and checking where they are used.
swarm/network/fetcher.gohas been removed and replaced withsingleflight.Group. Every time theNetstore.Getlooks for chunk that is not in our LocalStore, it uses thesingleflight.Groupin order to suppress duplicate retrievals for the same chunk.Each in-flight request for a chunk adds a
Fetchervalue to a map, so that theNetStore.Getreceives a signal that the chunk it requested has been delivered. The signal that a chunk is delivered and stored to the LocalStore is the closing of theFetcher.Deliveredchannel.NetStore.Putis responsible for removing fetchers and closing the delivered channel. If a fetcher item has been added to the fetchers map, then we must get that chunk and there is an interested party in the delivered signal.We need to add a timeout to the fetchers, because even though a chunk has been requested and an actor waits for it, doesn't mean it will be delivered.
handleOfferedHashesMsguses theNetstore.Hasto determine if it needs a chunk (while syncing). If a chunk is needed, then aFetcheris also created, so that we keep track while the current batch is being delivered.It is possible for a chunk to be both requested through a retrieve request and delivered through syncing independently, but there would always be only one
Fetcherand only oneDeliveredchannel for it so that interested parties are notified when the chunk is delivered and stored.// * first it tries to request explicitly from peers that are known to have offered the chunk- this part of the functionality ofRequestFromPeershas been removed. We no longer control the flow through values in thecontext.Context.RequestTimeouthas been split intoRequestTimeoutandFailedPeerSkipDelayas these have different meanings. All timeouts are now placed in the timeouts package, and have documentation.Added
LNetStore(we probably need a better name) - aNetStorewrapper needed for theLazyChunkReader.Found a bug in
OfferedHasheswhere we request the same chunk from multiple peers via theOfferedHashes/WantedHashesprotocol. In a future PR we will address it the following way: if we have requested a chunk, we have a fetcher for it, so subsequentOfferedHasheswon't have an effect.Solved bug where the interval passed to SubscriptionPull is exclusive, meaning we lose one chunk between batches.
Solved a bug with chunk deliveries described at Fix tracing for chunk delivery #1292 - chunks are delivered, but fetcher continues to make requests for the same chunk.
TODO TESTS:
streamer_test.gotestsTestDeliveryFromNodesis removed altogether, because we no longer blindly forward requests to our peers -FindPeerwas changed.TestRetrievalTestFileRetrievalTODO:
NICE TO HAVE:
timeouts.SearchTimeout- we can immediately call next peer.FindPeer- it has at least one new condition - not going out of depth - Add test forFindPeer#1362