Make InMemory object store track last modified time for each entry #3796

Weijun-H · 2023-03-02T21:20:05Z

Which issue does this PR close?

Closes #3782

Rationale for this change

Explain in #3782

What changes are included in this PR?

In struct InMemoryUpload, it can store btye with timestamp

Are there any user-facing changes?

gruuya

I'd argue that the copy/copy_if_not_exists methods should also alter the last modified timestamps if we're going to emulate local FS behavior:

$ touch test
$ date -r test
Fri Mar  3 09:32:25 CET 2023
$ cp test test_cp
$ date -r test_cp
Fri Mar  3 09:33:50 CET 2023

That said, rename also uses copy (and likewise for rename_if_not_exists), but it probably shouldn't affect the timestamp:

$ mv test test_mv
$ date -r test_mv
Fri Mar  3 09:32:25 CET 2023

tustvold

I think we should be returning this timestamp as the last_modified when we return ObjectMeta, and possibly verify that we are setting it consistently in all the mutating methods (such as copy)

tustvold · 2023-03-03T10:51:24Z

object_store/src/memory.rs

@@ -273,22 +278,23 @@ impl InMemory {
        }
    }

-    async fn get_bytes(&self, location: &Path) -> Result<Bytes> {
+    async fn get_bytes(&self, location: &Path) -> Result<(Bytes, DateTime<Utc>)> {


Suggested change

async fn get_bytes(&self, location: &Path) -> Result<(Bytes, DateTime<Utc>)> {

async fn entry(&self, location: &Path) -> Result<(Bytes, DateTime<Utc>)> {

tustvold · 2023-03-03T10:52:01Z

object_store/src/memory.rs

@@ -33,6 +33,8 @@ use std::sync::Arc;
 use std::task::Poll;
 use tokio::io::AsyncWrite;

+type StorageType = Arc<RwLock<BTreeMap<Path, (Bytes, DateTime<Utc>)>>>;


Suggested change

type StorageType = Arc<RwLock<BTreeMap<Path, (Bytes, DateTime<Utc>)>>>;

type Entry = (Bytes, DateTime<Utc>);

type StorageType = Arc<RwLock<BTreeMap<Path, Entry>>>;

tustvold · 2023-03-03T10:52:22Z

object_store/src/memory.rs

@@ -150,7 +155,7 @@ impl ObjectStore for InMemory {
        Ok(ObjectMeta {
            location: location.clone(),
            last_modified,


I think this should return the timestamp from the entry?

tustvold · 2023-03-03T10:52:42Z

object_store/src/memory.rs

@@ -181,7 +186,7 @@ impl ObjectStore for InMemory {
                Ok(ObjectMeta {
                    location: key.clone(),
                    last_modified,


This should be the value from the entry?

tustvold · 2023-03-03T10:52:55Z

object_store/src/memory.rs

@@ -225,7 +230,7 @@ impl ObjectStore for InMemory {
                let object = ObjectMeta {
                    location: k.clone(),
                    last_modified,


tustvold · 2023-03-03T12:26:32Z

object_store/src/memory.rs

@@ -273,22 +275,23 @@ impl InMemory {
        }
    }

-    async fn get_bytes(&self, location: &Path) -> Result<Bytes> {
+    async fn enty(&self, location: &Path) -> Result<(Bytes, DateTime<Utc>)> {


Suggested change

async fn enty(&self, location: &Path) -> Result<(Bytes, DateTime<Utc>)> {

async fn entry(&self, location: &Path) -> Result<(Bytes, DateTime<Utc>)> {

tustvold · 2023-03-03T15:52:23Z

object_store/src/memory.rs

@@ -238,13 +240,13 @@ impl ObjectStore for InMemory {
    }

    async fn copy(&self, from: &Path, to: &Path) -> Result<()> {
-        let data = self.get_bytes(from).await?;
+        let data = self.entry(from).await?;


I think this should probably generate a new last modified timestamp

like this self.storage.write().insert(to.clone(), (data.0, Utc::now()));?

tustvold · 2023-03-03T15:52:31Z

object_store/src/memory.rs

        self.storage.write().insert(to.clone(), data);
        Ok(())
    }

    async fn copy_if_not_exists(&self, from: &Path, to: &Path) -> Result<()> {
-        let data = self.get_bytes(from).await?;
+        let data = self.entry(from).await?;


tustvold · 2023-03-04T17:37:13Z

Thanks again

ursabot · 2023-03-04T17:42:24Z

Benchmark runs are scheduled for baseline = 6cd0917 and contender = 79518cf. 79518cf is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added the object-store Object Store Interface label Mar 2, 2023

Weijun-H changed the title ~~refactor: allow InMemoryUpload to store timestamp~~ Make InMemory object store track last modified time for each entry Mar 2, 2023

Weijun-H force-pushed the refactor-InMemory branch from 4ebc9bb to 7fd6fb1 Compare March 2, 2023 21:36

gruuya reviewed Mar 3, 2023

View reviewed changes

tustvold reviewed Mar 3, 2023

View reviewed changes

Weijun-H force-pushed the refactor-InMemory branch from 7fd6fb1 to c11ec79 Compare March 3, 2023 11:06

tustvold reviewed Mar 3, 2023

View reviewed changes

refactor: allow InMemoryUpload to store timestamp

697a504

Weijun-H force-pushed the refactor-InMemory branch from c11ec79 to 697a504 Compare March 3, 2023 12:45

tustvold approved these changes Mar 3, 2023

View reviewed changes

tustvold reviewed Mar 3, 2023

View reviewed changes

use new last modified timestamp

3f6a920

Weijun-H force-pushed the refactor-InMemory branch from aaaf49a to 3f6a920 Compare March 4, 2023 16:09

tustvold merged commit 79518cf into apache:master Mar 4, 2023

tustvold mentioned this pull request Mar 10, 2023

Make InMemory object store track last modified time for each entry #3782

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make InMemory object store track last modified time for each entry #3796

Make InMemory object store track last modified time for each entry #3796

Weijun-H commented Mar 2, 2023

gruuya left a comment

tustvold left a comment

tustvold Mar 3, 2023

tustvold Mar 3, 2023

tustvold Mar 3, 2023

tustvold Mar 3, 2023

tustvold Mar 3, 2023

tustvold Mar 3, 2023

tustvold Mar 3, 2023

Weijun-H Mar 3, 2023 •

edited

Loading

tustvold Mar 3, 2023

tustvold commented Mar 4, 2023

ursabot commented Mar 4, 2023

	async fn get_bytes(&self, location: &Path) -> Result<(Bytes, DateTime<Utc>)> {
	async fn entry(&self, location: &Path) -> Result<(Bytes, DateTime<Utc>)> {

	type StorageType = Arc<RwLock<BTreeMap<Path, (Bytes, DateTime<Utc>)>>>;
	type Entry = (Bytes, DateTime<Utc>);
	type StorageType = Arc<RwLock<BTreeMap<Path, Entry>>>;

	async fn enty(&self, location: &Path) -> Result<(Bytes, DateTime<Utc>)> {
	async fn entry(&self, location: &Path) -> Result<(Bytes, DateTime<Utc>)> {

Make InMemory object store track last modified time for each entry #3796

Make InMemory object store track last modified time for each entry #3796

Conversation

Weijun-H commented Mar 2, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

gruuya left a comment

Choose a reason for hiding this comment

tustvold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Weijun-H Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold commented Mar 4, 2023

ursabot commented Mar 4, 2023

Weijun-H Mar 3, 2023 •

edited

Loading