add est-download-speed to make data fetching timeout adjustable #35
Conversation
Signed-off-by: Merlin Ran <merlinran@gmail.com>
| fetchTimeout := DefaultDataURIFetchTimeout | ||
| if size != 0 { | ||
| // add baseline timeout for very small files | ||
| fetchTimeout = time.Minute + time.Duration(size/s.estDownloadSpeed)*time.Second |
There was a problem hiding this comment.
I considered making the fetch timeout 2X the value, but don't want to introduce some magic number. The default is small enough, and the help message asked the miners to be conservative.
| // DataURIFetchTimeout is the timeout used when fetching data uris. | ||
| DataURIFetchTimeout = time.Hour * 3 | ||
| // DefaultDataURIFetchTimeout is the timeout used when fetching data uris of unknown size. | ||
| DefaultDataURIFetchTimeout = time.Hour * 10 |
There was a problem hiding this comment.
extended the default now it's only used in the download command.
| @@ -491,7 +494,13 @@ func (s *Store) WriteDataURI(bidID auction.BidID, payloadCid, uri string) (strin | |||
| if _, err := f.Seek(0, 0); err != nil { | |||
| return "", fmt.Errorf("seeking file to the beginning: %v", err) | |||
| } | |||
| ctx, cancel := context.WithTimeout(s.ctx, DataURIFetchTimeout) | |||
| fetchTimeout := DefaultDataURIFetchTimeout | |||
| if size != 0 { | |||
There was a problem hiding this comment.
Oh ok, i see now why service/service.go:L302 works.
| // DefaultDataURIFetchTimeout is the timeout used when fetching data uris of unknown size. | ||
| DefaultDataURIFetchTimeout = time.Hour * 10 |
main.go
Outdated
| @@ -124,6 +124,12 @@ func init() { | |||
| DefValue: 24 * time.Hour, | |||
| Description: "The timeout before discarding deal with no progress", | |||
| }, | |||
| { | |||
| Name: "est-download-speed", | |||
| DefValue: "5MB", | |||
There was a problem hiding this comment.
I'd be more conservative here. The existing default was 3hs.
The usual CAR files are ~32GiB, and usually, the same miner receives a batch of things to download.. so more than one thing downloading at the same time. Maybe 1MB/s as default.
I believe we have much more room for improvement in all this download+import... like, a miner downloading 10 CAR files at the same time not only is a risk for download, but I'd imagine importing 10 CAR files later at the same time might nuke their lotus node.
We could always cap the amount of things done concurrently as I suggested some times.. but that's another feature that requires a bit more work.
There was a problem hiding this comment.
So for 1MB/s would be 9hs. If is downloading 3 CAR files at the same time, is an avg of 3hs per CAR. (all hand-wavy...)
Maybe too conservative... but around there, could be a bit more.
There was a problem hiding this comment.
1MB sounds good. it is what I originally set. good point on concurrent downloads. Maybe just limit MaxDataURIFetchConcurrency to 3? 10 concurrent downloads won't make sense if the bottleneck is bandwidth.
There was a problem hiding this comment.
Yep, I believe we need that flag.
There was a problem hiding this comment.
At some point might be good to stop bidding if you already have X things waiting for download [just sharing thought for a next feature]
There was a problem hiding this comment.
It's an existing constant we can simply change right now.
There was a problem hiding this comment.
Oh, right didn't recall. LGTM to switch to 3.
Signed-off-by: Merlin Ran <merlinran@gmail.com>
Adding this seemingly useful feature, while I'm here.
Resolves #33