Skip to content
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

fix: object store http header last modified #4834

Merged
merged 5 commits into from
Sep 19, 2023

Conversation

universalmind303
Copy link
Contributor

Which issue does this PR close?

Closes #4831

Rationale for this change

object store 0.7.0 caused breaking changes in fetching from endpoints without last_modified

What changes are included in this PR?

If the last-modified header is not present, it will default to the epoch.

Are there any user-facing changes?

@github-actions github-actions bot added the object-store Object Store Interface label Sep 18, 2023
@tustvold
Copy link
Contributor

I need to think on this, it might be we need to loosen the requirements for HttpStore, ideally as a configurable option.

However, as written this PR will loosen the requirements for things like S3, etc... which I'm not sure is a good idea

@universalmind303
Copy link
Contributor Author

However, as written this PR will loosen the requirements for things like S3, etc... which I'm not sure is a good idea

That makes sense. I just pushed up a refactor that changes header_meta to take in a new config struct that allows users to specify which fields are required. (last_modified, etag) being the two that are optional.

This should leave the other object stores like S3 unaffected.

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, I tried to apply the changes to avoid a round-trip but it would appear your PR blocks edits by maintainers, so I've added them as suggestions instead

object_store/src/client/header.rs Outdated Show resolved Hide resolved
object_store/src/client/header.rs Outdated Show resolved Hide resolved
object_store/src/client/header.rs Outdated Show resolved Hide resolved
object_store/src/client/header.rs Show resolved Hide resolved
universalmind303 and others added 3 commits September 19, 2023 08:41
Co-authored-by: Raphael Taylor-Davies <[email protected]>
Co-authored-by: Raphael Taylor-Davies <[email protected]>
Co-authored-by: Raphael Taylor-Davies <[email protected]>
@tustvold tustvold merged commit a03ce56 into apache:master Sep 19, 2023
13 checks passed
ryanaston pushed a commit to segmentio/arrow-rs that referenced this pull request Nov 6, 2023
* fix: object store http header last modified

* refactor: make headermeta configurable on required fields

* Update object_store/src/client/header.rs

Co-authored-by: Raphael Taylor-Davies <[email protected]>

* Update object_store/src/client/header.rs

Co-authored-by: Raphael Taylor-Davies <[email protected]>

* Update object_store/src/client/header.rs

Co-authored-by: Raphael Taylor-Davies <[email protected]>

---------

Co-authored-by: Raphael Taylor-Davies <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
object-store Object Store Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

object-store: http-store fails when url doesn't have last-modified header on 0.7.0
2 participants