[refactor] - Add DataOrErr #3520
Conversation
pkg/handlers/default.go
Outdated
| func (h *defaultHandler) measureLatencyAndHandleErrors(start time.Time, err error) { | ||
| func (h *defaultHandler) measureLatencyAndHandleErrors( | ||
| ctx logContext.Context, | ||
| start time.Time, | ||
| err error, | ||
| dataErrChan chan DataOrErr, | ||
| ) { |
There was a problem hiding this comment.
I'm not super familiar with this area of the codebase, but it's starting to smell like this method is doing a little too much.
Am I reading this right, that we're now writing to the dataErrChan instead of just recording metrics? (side nit: make dataErrChan a write-only parameter).
There was a problem hiding this comment.
I agree. I can create a follow-up PR to break this function into smaller parts. You're correct—we need to pass all errors to dataErrChan instead of just logging them and moving on, as we did before.
pkg/handlers/handlers.go
Outdated
| func newFileReader(r io.Reader) (fileReader, error) { | ||
| var fReader fileReader | ||
|
|
||
| // The caller is responsible for closing the reader when it is no longer needed. | ||
| func newFileReader(r io.Reader) (fReader fileReader, err error) { | ||
| // To detect the MIME type of the input data, we need a reader that supports seeking. | ||
| // This allows us to read the data multiple times if necessary without losing the original position. | ||
| // We use a BufferedReaderSeeker to wrap the original reader, enabling this functionality. | ||
| fReader.BufferedReadSeeker = iobuf.NewBufferedReaderSeeker(r) | ||
|
|
||
| mime, err := mimetype.DetectReader(fReader) | ||
| // If an error occurs during MIME type detection, it is important we close the BufferedReaderSeeker | ||
| // to release any resources it holds (checked out buffers or temp file). | ||
| defer func() { | ||
| if err != nil { | ||
| if closeErr := fReader.Close(); closeErr != nil { | ||
| err = fmt.Errorf("%w; error closing reader: %w", err, closeErr) | ||
| } | ||
| } | ||
| }() | ||
|
|
||
| var mime *mimetype.MIME | ||
| mime, err = mimetype.DetectReader(fReader) | ||
| if err != nil { | ||
| return fReader, fmt.Errorf("unable to detect MIME type: %w", err) | ||
| } | ||
| fReader.mime = mime | ||
|
|
||
| // Reset the reader to the beginning because DetectReader consumes the reader. | ||
| if _, err := fReader.Seek(0, io.SeekStart); err != nil { | ||
| if _, err = fReader.Seek(0, io.SeekStart); err != nil { |
There was a problem hiding this comment.
These changes look unrelated to DataOrErr, were they accidentally added to this PR?
There was a problem hiding this comment.
Ah, yes—these changes were part of an earlier PR that I split into three separate PRs as requested. This felt like the most appropriate PR to include the change originally in the initial PR."
| if br.buf == nil { | ||
| br.buf = br.bufPool.Get() | ||
| } | ||
|
|
There was a problem hiding this comment.
Same question here about this being unrelated to DataOrErr.
Description:
This PR addresses the second point in this comment.
Checklist:
make test-community)?make lintthis requires golangci-lint)?