-
Notifications
You must be signed in to change notification settings - Fork 50
FSTree buffered HEAD #3810
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
Open
cthulhu-rider
wants to merge
1
commit into
master
Choose a base branch
from
fstree-head-buffer
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
FSTree buffered HEAD #3810
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,81 @@ func (t *FSTree) Head(addr oid.Address) (*object.Object, error) { | |
| return obj, nil | ||
| } | ||
|
|
||
| // ReadHeader reads header of the referenced object from t into buffer provided | ||
| // by getBuffer. Returns number of bytes read. | ||
| // | ||
| // If object is missing, ReadHeader returns [apistatus.ErrObjectNotFound]. | ||
| // | ||
| // The buffer must have 2*[object.MaxHeaderLen] bytes len at least. | ||
| func (t *FSTree) ReadHeader(addr oid.Address, getBuffer func() []byte) (int, error) { | ||
| p := t.treePath(addr) | ||
|
|
||
| f, err := os.Open(p) | ||
| if err != nil { | ||
| if errors.Is(err, fs.ErrNotExist) { | ||
| return 0, logicerr.Wrap(apistatus.ErrObjectNotFound) | ||
| } | ||
| return 0, fmt.Errorf("read file %q: %w", p, err) | ||
| } | ||
| defer f.Close() | ||
|
|
||
| buf := getBuffer() | ||
| if len(buf) < 2*object.MaxHeaderLen { | ||
| return 0, fmt.Errorf("too short buffer %d bytes", len(buf)) | ||
| } | ||
|
|
||
| from, to, rem, err := t.readHeader(buf, addr.Object(), f) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
|
|
||
| // following mostly copied from readHeaderAndPayload() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't want to duplicate the code. |
||
|
|
||
| compressed := t.IsCompressed(buf[from:to]) | ||
| if !compressed { | ||
| return copy(buf, buf[from:to]), nil | ||
| } | ||
|
|
||
| if to-from < object.MaxHeaderLen { | ||
| dec, err := t.DecompressForce(buf[from:to]) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("decompress initial data: %w", err) | ||
| } | ||
| if len(dec) > len(buf) { | ||
| return 0, fmt.Errorf("decompressed data len %d overflows buffer len %d", len(dec), len(buf)) | ||
| } | ||
|
|
||
| return copy(buf, dec), nil | ||
| } | ||
|
|
||
| var r io.Reader | ||
| if rem < 0 { // non-combined | ||
| r = io.MultiReader(bytes.NewReader(buf[from:to]), f) | ||
| } else if rem == 0 { | ||
| r = bytes.NewReader(buf[from:to]) | ||
| } else { | ||
| r = io.MultiReader(bytes.NewReader(buf[from:to]), io.LimitReader(f, rem)) | ||
| } | ||
|
|
||
| decoder, err := zstd.NewReader(r) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("zstd decoder: %w", err) | ||
| } | ||
| defer decoder.Close() | ||
|
|
||
| decBuf := make([]byte, object.MaxHeaderLen) | ||
|
|
||
| n, err := decoder.Read(decBuf) | ||
| if err != nil && !errors.Is(err, io.EOF) { | ||
| return 0, fmt.Errorf("zstd read: %w", err) | ||
| } | ||
| if n > len(buf) { | ||
| return 0, fmt.Errorf("decompressed data len %d overflows buffer len %d", n, len(buf)) | ||
| } | ||
|
|
||
| return copy(buf, decBuf[:n]), nil | ||
| } | ||
|
|
||
| // getObjectStream reads an object from the storage by address as a stream. | ||
| // It returns the object with header only, and a reader for the payload. | ||
| func (t *FSTree) getObjectStream(addr oid.Address) (*object.Object, io.ReadSeekCloser, error) { | ||
|
|
@@ -58,17 +133,39 @@ func (t *FSTree) getObjectStream(addr oid.Address) (*object.Object, io.ReadSeekC | |
| // The caller is responsible for closing the returned io.ReadCloser if it is not nil. | ||
| func (t *FSTree) extractHeaderAndStream(id oid.ID, f *os.File) (*object.Object, io.ReadSeekCloser, error) { | ||
| buf := make([]byte, 2*object.MaxHeaderLen) | ||
|
|
||
| from, to, rem, err := t.readHeader(buf, id, f) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| if rem <= 0 { | ||
| return t.readHeaderAndPayload(f, buf[from:to]) | ||
| } | ||
|
|
||
| rc := struct { | ||
| io.Reader | ||
| io.Closer | ||
| }{Reader: io.LimitReader(f, rem), Closer: f} | ||
|
|
||
| return t.readHeaderAndPayload(rc, buf[from:to]) | ||
| } | ||
|
|
||
| // reads header of object by id from f into buf. Returns first and last | ||
| // bytes in buf containing the result. Third return is the number of bytes left | ||
| // it f is combined (negative if not combined). | ||
| func (t *FSTree) readHeader(buf []byte, id oid.ID, f *os.File) (int, int, int64, error) { | ||
| n, err := io.ReadFull(f, buf[:object.MaxHeaderLen]) | ||
| if err != nil && !errors.Is(err, io.EOF) && !errors.Is(err, io.ErrUnexpectedEOF) { | ||
| return nil, f, err | ||
| return 0, 0, 0, err | ||
| } | ||
| if n < combinedDataOff { | ||
| return t.readHeaderAndPayload(f, buf[:n]) | ||
| return 0, n, -1, nil | ||
| } | ||
|
|
||
| thisOID, l := parseCombinedPrefix(buf) | ||
| if thisOID == nil { | ||
| return t.readHeaderAndPayload(f, buf[:n]) | ||
| return 0, n, -1, nil | ||
| } | ||
|
|
||
| offset := combinedDataOff | ||
|
|
@@ -78,44 +175,40 @@ func (t *FSTree) extractHeaderAndStream(id oid.ID, f *os.File) (*object.Object, | |
| if n < size { | ||
| _, err = io.ReadFull(f, buf[n:size]) | ||
| if err != nil { | ||
| return nil, f, fmt.Errorf("read up to size: %w", err) | ||
| return 0, 0, 0, fmt.Errorf("read up to size: %w", err) | ||
| } | ||
| } | ||
|
|
||
| f := io.ReadCloser(f) | ||
| if buffered := uint32(size - offset); l > buffered { | ||
| f = struct { | ||
| io.Reader | ||
| io.Closer | ||
| }{Reader: io.LimitReader(f, int64(l-buffered)), Closer: f} | ||
| return offset, size, int64(l - buffered), nil | ||
| } | ||
|
|
||
| return t.readHeaderAndPayload(f, buf[offset:size]) | ||
| return offset, size, 0, nil | ||
| } | ||
|
|
||
| offset += int(l) | ||
| if n-offset < combinedDataOff { | ||
| if offset > n { | ||
| _, err = f.Seek(int64(offset-n), io.SeekCurrent) | ||
| if err != nil { | ||
| return nil, f, err | ||
| return 0, 0, 0, err | ||
| } | ||
| } | ||
| n = copy(buf, buf[min(offset, n):n]) | ||
| offset = 0 | ||
| k, err := io.ReadFull(f, buf[n:n+object.MaxHeaderLen]) | ||
| if err != nil && !errors.Is(err, io.EOF) && !errors.Is(err, io.ErrUnexpectedEOF) { | ||
| return nil, f, fmt.Errorf("read full: %w", err) | ||
| return 0, 0, 0, fmt.Errorf("read full: %w", err) | ||
| } | ||
| if k == 0 { | ||
| return nil, f, fmt.Errorf("file was found, but this object is not in it: %w", io.ErrUnexpectedEOF) | ||
| return 0, 0, 0, fmt.Errorf("file was found, but this object is not in it: %w", io.ErrUnexpectedEOF) | ||
| } | ||
| n += k | ||
| } | ||
|
|
||
| thisOID, l = parseCombinedPrefix(buf[offset:]) | ||
| if thisOID == nil { | ||
| return nil, f, errors.New("malformed combined file") | ||
| return 0, 0, 0, errors.New("malformed combined file") | ||
| } | ||
|
|
||
| offset += combinedDataOff | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
How about
ReadHeaderInto(addr oid.Address, buf []byte) (int, error)which is very close to https://pkg.go.dev/io#Reader (or returning([]byte, error)like https://pkg.go.dev/math/big#Int.FillBytes or https://pkg.go.dev/encoding#TextAppender (although that one appends)). Buffering is still completely out of FSTree scope, but there are no functions involved.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.
i thought about
io.Reader-like interface, others are not appropriate for current needsit's good, but idle allocs when file cannot be opened (e.g. 404) concern me. Dynamic allocation after opening the file is what we have now. Buffer's parameterization requires its preallocation. It may remain unused. Buffer pool (TBD) will allow to potentially reuse the buffer, but there can also be a lot of erroneous calls
what do u think?
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.
I wouldn't expect a lot of 404 at this level. Normally it's filtered through metabase and it knows what we have or not correctly.
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.
in dont mind to accept this within shard. But a similar issue will arise at higher levels: who manages buffers - service or storage engine?
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.
You have to pass something down anyway, whether it's a callback or a slice. Then you can consider this to be a price of a call (successful or not). Also, if you're to manage response message buffers you can arrange a slice prefixed with HEAD (or GET) reply wrapper (or some space for it) and then pass a subslice for header itself here.
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 the only thing that is rly being discussed. Im proposing a callback. Is there any real reason to choose another option?
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.
Much simpler interface. Consider addr/buf matching for example, you have to resolve it in callback instead of having some direct result (where to apply that
int?). Consider debugging, where does this buffer come from really? Can I check its contents at shard/engine level (where callers won't even see it)?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.
in practice, the buffer is allocated once and reused. Callback simply allows to allocate it when it is really needed
i understand the interface will be a bit simpler. But idle creation in some cases and closeness to the current behavior are still relevant to me. I don't see any real reason to change this behavior yet