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

feat: add etag for objectMeta #3937

Merged
merged 5 commits into from
Apr 3, 2023
Merged

Conversation

Weijun-H
Copy link
Member

@Weijun-H Weijun-H commented Mar 24, 2023

Which issue does this PR close?

Closes #2240

Rationale for this change

Add Etag for objectMeta as a unique identifier, which is a hash value by last_modified.

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the object-store Object Store Interface label Mar 24, 2023
@Weijun-H Weijun-H force-pushed the add-etag-object-store branch 3 times, most recently from cfe988b to e72fbbe Compare March 24, 2023 23:22
@tustvold
Copy link
Contributor

For the actual stores, like S3, the etag is part of the response. Using a manual hash is just for in memory and filesystem where this data isn't stored

@tustvold tustvold added the api-change Changes to the arrow API label Mar 25, 2023
@Weijun-H Weijun-H force-pushed the add-etag-object-store branch 6 times, most recently from 3632169 to b07fe31 Compare March 25, 2023 14:30
@tustvold
Copy link
Contributor

The emulator tests appear to be failing for this?

@Weijun-H Weijun-H force-pushed the add-etag-object-store branch 2 times, most recently from fa15364 to 14198da Compare March 28, 2023 21:44
@Weijun-H
Copy link
Member Author

Weijun-H commented Mar 28, 2023

The emulator tests appear to be failing for this?

---- http::tests::http_test stdout ----
thread 'http::tests::http_test' panicked at 'called `Result::unwrap()` on an `Err` value: Generic { store: "HTTP", source: InvalidPropFind { source: Custom("missing field `getetag`") } }', object_store/src/lib.rs:1203:62

getetag should work, but it failed. I am not idea how to change it.

[13.6](https://datatracker.ietf.org/doc/html/rfc2518#section-13.6) getetag Property

   Name:       getetag
   Namespace:  DAV:
   Purpose:    Contains the ETag header returned by a GET without
   accept headers.
   Description: The getetag property MUST be defined on any DAV
   compliant resource that returns the Etag header.
   Value:      entity-tag  ; defined in [section 3.11 of [RFC2068]](https://datatracker.ietf.org/doc/html/rfc2068#section-3.11)

   <!ELEMENT getetag (#PCDATA) >

@tustvold
Copy link
Contributor

It may not be supported, we may need to make the ETag optional 🤔

object_store/src/lib.rs Outdated Show resolved Hide resolved
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.

Looks good to me, thank you, as this is a breaking change I'm going to hold off on merging this until I've cut the next point release which I intend to do this wewk

object_store/Cargo.toml Outdated Show resolved Hide resolved
@tustvold tustvold merged commit a9b1120 into apache:master Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change Changes to the arrow API object-store Object Store Interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ETag to ObjectMeta
2 participants