-
Notifications
You must be signed in to change notification settings - Fork 139
feat: puffin Reader and Writer #676
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Shreyas220 <shreyas.ny@gmail.com>
Signed-off-by: Shreyas220 <shreyas.ny@gmail.com>
Signed-off-by: Shreyas220 <shreyas.ny@gmail.com>
Signed-off-by: Shreyas220 <shreyas.ny@gmail.com>
|
Hey 👋 @zeroshade , @twuebi would appreciate a review |
zeroshade
left a comment
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.
Thanks for starting this up! I've been wanting to get a puffin reader/writer in here for a while now. I've gone through with a bunch of questions / requests etc. But this also needs to have some tests and potentially examples if possible!
Thanks again!
puffin/puffin_writer.go
Outdated
|
|
||
| // SetProperties merges the provided properties into the file-level properties | ||
| // written to the footer. Can be called multiple times before Finish. | ||
| func (w *PuffinWriter) SetProperties(props map[string]string) error { |
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.
should we have a way to remove properties also? Or a way to clear the current properties?
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.
Since this is cumulative, maybe it should be renamed as "AddProperties" ?
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.
Added ClearProperties() method .
is there a requirement for removing a specific property ?
puffin/puffin_reader.go
Outdated
| // ReadRange reads a raw byte range from the blob data region. | ||
| // The footer must be read first to establish bounds checking. | ||
| func (r *PuffinReader) ReadRange(offset, length int64) ([]byte, error) { |
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.
what is the actual use of this function? When would a user want to use this?
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.
Deletion vector in manifests include offset and length into Puffin file. can directly read that range
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 implement ReadAt instead of ReadRange?
Signed-off-by: Shreyas220 <shreyas.ny@gmail.com>
Signed-off-by: Shreyas220 <shreyas.ny@gmail.com>
48a86d6 to
cd19142
Compare
Signed-off-by: Shreyas220 <shreyas.ny@gmail.com>
c54c147 to
3e3ea83
Compare
for #589