Refactor error handling on read requests

[passaro]

Description of change

Minor refactor to simplify how errors returned from Prefetch::read() are handled. It implements the error conversion in the fs::error module, to align it to errors on the write path.

Preliminary work for #644

Does this change impact existing behavior?

No change in behavior.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).

[sauraank]

LGTM. I had one question. Why are we directly returning Bytes from prefetcher rather than converting them from ChecksummedBytes at the fs level?

[passaro]

LGTM. I had one question. Why are we directly returning Bytes from prefetcher rather than converting them from ChecksummedBytes at the fs level?

• the prefetcher can also return a PrefetchReadError::Integrity (in case the integrity checks failed at some other point in the pipeline), so it is simpler to merge the two and return an already validated Bytes
• Before the change, we were immediately validating the ChecksummedBytes and extracting the Bytes anyway, so this does not actually moves when the validation happens.

[jamesbornholt]

I kind of think we might prefer to keep the prefetcher returning ChecksummedBytes. Today we just convert immediately, but there's nothing stopping future us from inserting more work between the prefetcher and the FUSE response, and the type at least makes that decision clear.

[passaro]

@jamesbornholt, @sauraank, I reverted the change to the return type of read. It brings back 2 routes to get a form of integrity error, but overall I agree we want to propagate our "checked" type as much as possible, even if right now we immediately extract the buffer.

[jamesbornholt]

Ok, but now I'm not sure why we need this change — does moving the error parsing logic out into a From impl buy us anything? I reverted this after checking out #644 and it didn't seem to break anything.

[passaro]

Ok, but now I'm not sure why we need this change — does moving the error parsing logic out into a From impl buy us anything? I reverted this after checking out #644 and it didn't seem to break anything.

Fair question. You're right, this is not really tied to #644 anymore, but I do see moving the PrefetchReadError mapping logic to the error module, as done for UploadWriteError, as a marginal improvement.

[dannycjones]

lgtm!