feat: replace CarOffsetWriter with car.NewCarV1StreamReader#693
feat: replace CarOffsetWriter with car.NewCarV1StreamReader#693
Conversation
rvagg
force-pushed
the
rvagg/replace-cow-with-selective-car-with-skip
branch
from
d139989 to
6f65d7a
Compare
|
I think it should be possible to replicate the I just don't know about parallel requests for the same |
|
Instead of making the stream readers somehow safe for multiple concurrent users, which will be tough given the semantics of read vs seek, it should be pretty cheap to be able to do a clone() to make a deep copy of metadata for a new user, and then maybe also not too bad to have a |
dirkmc
left a comment
dirkmc
left a comment
There was a problem hiding this comment.
It would be great to get rid of CarOffsetWriter! 👍
| cancelContent := func(parentCtx context.Context) (err error) { | ||
| cancelComplete := make(chan struct{}, 1) | ||
| go func() { | ||
| // calling a Seek() on the SkipWriterReaderSeeker from go-car will cancel | ||
| // any existing writer and block until that cancel is complete | ||
| if closeErr := closer.Close(); closeErr != nil && !errors.Is(closeErr, context.Canceled) { | ||
| // context.Canceled can come up through the http Request from a socket close, ignore it | ||
| err = closeErr | ||
| } | ||
| cancelComplete <- struct{}{} | ||
| }() | ||
| select { | ||
| case <-parentCtx.Done(): | ||
| return parentCtx.Err() | ||
| case <-cancelComplete: | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
nit: I'd suggest moving the cancelContent function inside of sendCar as it's not used anywhere else
| bic := s.bicm.Get(authVal.PayloadCid) | ||
| err = s.serveContent(w, r, authToken, authVal, bic) | ||
| s.bicm.Unref(authVal.PayloadCid, err) | ||
| _ = s.serveContent(w, r, authToken, authVal) |
There was a problem hiding this comment.
Maybe we should log this error somewhere?
|
@rvagg what's the current state of this? It sounded like there were some challenges with the go-car updates but following the threads in the related issues wasn't quite clear. I'd like to understand what the blocker on this is so we can sort out next steps. |
|
Closing as out of date |
4 participants
WIP:
BlockInfoCache, so this version will re-load all the blocks during a resumption - it should do it properly, but the existing version keeps a cache of the block's position in the CAR and all of the links in the block so we can take shortcuts withmerkledag.Walk; we need to consider if/how we support that. Perhaps it's just a matter of holding onto the reader and reusing it, or holding onto some part of the reader? Since the newTraverseResumershould take care of these concerns for us.