Generate `ETag`s for `InMemory` and `LocalFileSystem` (#4879) by tustvold · Pull Request #4922 · apache/arrow-rs

tustvold · 2023-10-11T20:19:49Z

Which issue does this PR close?

Part of apache/arrow-rs-object-store#129

Rationale for this change

Implements ETag for InMemory using a simple monotonic counter.

What changes are included in this PR?

Are there any user-facing changes?

tustvold · 2023-10-11T21:41:45Z

object_store/src/local.rs

+
+#[cfg(not(unix))]
+fn get_inode(metadata: &Metadata) -> u64 {
+    0


The inode isn't strictly necessary, it is still a pretty strong ETag without it, but on Unix systems we might as well make use of it.

A happy consequence of the way most operations create a new file and then link it into place, is that the inode is actually a pretty good change detector

I don't understand this comment. If the inode isn't needed, then what value does including it add?

Either modification time and size (along with path) uniquely determines the result, or it isn't...

Perhaps the inode can detect file modifications that do not change the modification timestamp (some sort of symlink shenanigans, perhaps 🤔 )

In any event I think this "inode is not necessary" context should be encoded as comments in the source code

I see you have clarified the comment that inode is used to be more resistent to collisions -- sounds good to me

crepererum

Minor naming nitpick.

crepererum · 2023-10-16T14:35:12Z

object_store/src/lib.rs

-                    source: format!("{} >= {}", date, last_modified).into(),
+    ///
+    /// <https://datatracker.ietf.org/doc/html/rfc7232#section-6>
+    fn test(&self, meta: &ObjectMeta) -> Result<()> {


Suggested change

fn test(&self, meta: &ObjectMeta) -> Result<()> {

fn check_modification_conditions(&self, meta: &ObjectMeta) -> Result<()> {

"test" is rather non-descriptive and is easily confused w/ unit tests.

alamb

Thank you @tustvold -- the Etag feature is getting quite cool and complete.

I had two major comments:

etag test coverage

It seems like there is no test coverage of the etag generation functionality or did I miss that? When I say "no coverage" I mean we could remove all this code and all the tests would keep passing

etag documentation

Also, it seems like etags in general are not well documented in the object_store crate: https://docs.rs/object_store/latest/object_store/?search=etag

I suggest that we add documentation:

etags are a supported feature of the crate (perhaps a Etag section with brief introduction and example of how to use them, along with which object store adapters support them)
Improve https://docs.rs/object_store/latest/object_store/struct.ObjectMeta.html#structfield.e_tag to refer back to the example, and pointing at check_preconditions)?

I think the tests should be done prior to merge. The documentation is not strictly necessary

alamb · 2023-10-16T19:11:23Z

object_store/src/local.rs

+
+#[cfg(not(unix))]
+fn get_inode(metadata: &Metadata) -> u64 {
+    0


I don't understand this comment. If the inode isn't needed, then what value does including it add?

Either modification time and size (along with path) uniquely determines the result, or it isn't...

Perhaps the inode can detect file modifications that do not change the modification timestamp (some sort of symlink shenanigans, perhaps 🤔 )

alamb · 2023-10-16T19:11:54Z

object_store/src/local.rs

+
+#[cfg(not(unix))]
+fn get_inode(metadata: &Metadata) -> u64 {
+    0


In any event I think this "inode is not necessary" context should be encoded as comments in the source code

alamb · 2023-10-16T19:13:52Z

object_store/src/memory.rs

+    storage: SharedStorage,
+}
+
+type Entry = (Bytes, DateTime<Utc>, usize);


I think we should document what this triple is (specifically that the usize is an Etag)

alamb · 2023-10-16T19:21:23Z

object_store/src/lib.rs

+        let last_modified = meta.last_modified;
+
+        if let Some(m) = &self.if_match {
+            if m != "*" && m.split(',').map(str::trim).all(|x| x != etag) {


Can you please add an explicit mention of * handling as well as support for the , handling to the docstrings (so someone can see it on docs.rs)?

alamb · 2023-10-16T19:24:14Z

object_store/src/lib.rs

-    // DELETE nonexisting location
-    // PUT overwriting
+    #[test]
+    fn test_get_options() {


This is really a test of check_preconditions isn't it?

Suggested change

fn test_get_options() {

fn test_pre_conditions() {

tustvold · 2023-10-16T20:02:09Z

object_store/src/lib.rs

I opted to make these tests "mandatory" as now all stores should support them

alamb

Thank you @tustvold -- looks great to me

alamb · 2023-10-17T11:24:58Z

object_store/src/local.rs

+
+#[cfg(not(unix))]
+fn get_inode(metadata: &Metadata) -> u64 {
+    0


I see you have clarified the comment that inode is used to be more resistent to collisions -- sounds good to me

…#4922) * Support ETag in InMemory (#4879) * Add LocalFileSystem Etag * Review feedback * Review feedback

Support ETag in InMemory (#4879)

4160e96

github-actions bot added the object-store label Oct 11, 2023

Add LocalFileSystem Etag

8488509

tustvold changed the title ~~Support ETag in InMemory (#4879)~~ Support ETag in InMemory and LocalFileSystem (#4879) Oct 11, 2023

tustvold commented Oct 11, 2023

View reviewed changes

crepererum approved these changes Oct 16, 2023

View reviewed changes

Review feedback

534a7cc

alamb changed the title ~~Support ETag in InMemory and LocalFileSystem (#4879)~~ Generate ETags for InMemory and LocalFileSystem (#4879) Oct 16, 2023

alamb reviewed Oct 16, 2023

View reviewed changes

tustvold commented Oct 16, 2023

View reviewed changes

Review feedback

8aad50d

tustvold force-pushed the support-etag-in-memory branch from 9d0a02d to 8aad50d Compare October 16, 2023 20:05

alamb approved these changes Oct 17, 2023

View reviewed changes

tustvold merged commit ab87abd into apache:master Oct 17, 2023

tustvold mentioned this pull request Oct 23, 2023

Improve object_store docs #4978

Merged

alamb pushed a commit to alamb/arrow-rs that referenced this pull request Mar 20, 2025

Generate ETags for InMemory and LocalFileSystem (#4879) (apache…

54270c7

…#4922) * Support ETag in InMemory (#4879) * Add LocalFileSystem Etag * Review feedback * Review feedback

	fn test(&self, meta: &ObjectMeta) -> Result<()> {
	fn check_modification_conditions(&self, meta: &ObjectMeta) -> Result<()> {

Comments

Conversation

tustvold commented Oct 11, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

tustvold Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crepererum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

etag test coverage

etag documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tustvold Oct 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tustvold Oct 11, 2023 •

edited

Loading

alamb left a comment •

edited

Loading

tustvold Oct 16, 2023 •

edited

Loading