-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Streaming functionality #4
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is mostly fine, but I would like to retain the existing functionality.
carbites.go
Outdated
@@ -29,7 +29,7 @@ func Split(in io.Reader, targetSize int, s Strategy) (Splitter, error) { | |||
case Simple: | |||
return NewSimpleSplitter(in, targetSize) | |||
case Treewalk: | |||
return NewTreewalkSplitter(in, targetSize) | |||
return nil, fmt.Errorf("treewalk strategy caches the entier CAR, which is not allowed due to memory considerations") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please allow this? It should be up to the user to determine if they are ok to buffer in memory. I think io.Reader
isn't necessarily buffered anyway...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now this code path doesn't get accessed anyway, which is why I removed it.
In main.go
we first check for and handle treewalk strategy separately -- https://github.com/alanshaw/go-carbites/blob/main/cmd/main.go#L36 which means in carbites.go
we will never have case Treewalk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh i guess you mean because carbites.Split is public. Yeah, that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This is built on top of #3 so we can ignore that one.
This adds the ability to stream in a car file: