Update CRT `HeadersError` enum to include header name #1205

dannycjones · 2024-12-20T17:37:25Z

We recently saw an error in #1199 where "Header not found" was emitted, but its really unclear what header was missing.

2024-12-12T18:33:59.379478Z  WARN flush{req=1609 ino=2 fh=1 pid=29257 name="testfile_100M.bin"}: mountpoint_s3::fuse: flush failed: put failed: put request failed: Client error: Internal S3 client error: Header not found

This change updates the HeadersError::HeaderNotFound enum variant to contain a copy of the header name, such that error messages can emit it for debugging purposes.

It may make more sense to have all the header names we use statically defined somewhere, such that we could include a static reference to the header and avoid allocating for an error message. However, we don't expect there to be any performance regression introduced by this change. This move to static values could be made later.

Does this change impact existing behavior?

Header not found and invalid header value errors will now include the header name when printing the error message.

The enum variants change meaning any code using the enum may be impacted.

Does this change need a changelog entry?

Not for Mountpoint itself. I have added a change log entry to mountpoint-s3-crt since it is a breaking API change.

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

Signed-off-by: Daniel Carl Jones <[email protected]>

passaro

LGTM!

mountpoint-s3-crt/src/http/request_response.rs

Signed-off-by: Daniel Carl Jones <[email protected]>

We recently saw an error in awslabs#1199 where "Header not found" was emitted, but its really unclear what header was missing. 2024-12-12T18:33:59.379478Z WARN flush{req=1609 ino=2 fh=1 pid=29257 name="testfile_100M.bin"}: mountpoint_s3::fuse: flush failed: put failed: put request failed: Client error: Internal S3 client error: Header not found This change updates the `HeadersError::HeaderNotFound` enum variant to contain a copy of the header name, such that error messages can emit it for debugging purposes. It may make more sense to have all the header names we use statically defined somewhere, such that we could include a static reference to the header and avoid allocating for an error message. However, we don't expect there to be any performance regression introduced by this change. This move to static values could be made later. ### Does this change impact existing behavior? Header not found and invalid header value errors will now include the header name when printing the error message. The enum variants change meaning any code using the enum may be impacted. ### Does this change need a changelog entry? Not for Mountpoint itself. I have added a change log entry to `mountpoint-s3-crt` since it is a breaking API change. --- 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)](https://developercertificate.org/). --------- Signed-off-by: Daniel Carl Jones <[email protected]>

dannycjones added the performance PRs to run benchmarks on label Dec 20, 2024

dannycjones requested a review from passaro December 20, 2024 17:37

dannycjones had a problem deploying to PR benchmarks December 20, 2024 17:37 — with GitHub Actions Failure