mountpoint-s3 icon indicating copy to clipboard operation
mountpoint-s3 copied to clipboard

Published 20 hours ago • verified publisher icon awslabs

Refactor error handling on read requests

Open passaro opened this issue 1 year ago • 5 comments

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).

passaro avatar Feb 13 '24 16:02 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.

passaro avatar Feb 15 '24 13:02 passaro

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.

jamesbornholt avatar Feb 19 '24 17:02 jamesbornholt

@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.

passaro avatar Mar 11 '24 10:03 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.

jamesbornholt avatar Mar 11 '24 18:03 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.

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.

passaro avatar Mar 13 '24 13:03 passaro