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

Implement directory atime,ctime,mtime #100

Merged
merged 6 commits into from
Feb 27, 2023
Merged

Implement directory atime,ctime,mtime #100

merged 6 commits into from
Feb 27, 2023

Conversation

sauraank
Copy link
Contributor

Changed the directory mtime, ctime, atime to the instant filesystem is mounted, i.e., when the superblock is created. Not sure how to check if its the right current time. Modified the test to include utc_now() in test, but dont think it solves the purpose.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…tem is mounted.

Signed-off-by: Ankit Saurabh <sauraank@amazon.co.uk>
Copy link
Contributor

@dannycjones dannycjones left a comment

Choose a reason for hiding this comment

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

We want to broaden these changes to cover all directories and not just the root Inode.

Regarding testing, maybe we can perform a range check on the timestamp returned. We might say the date is within 5 seconds of a given timestamp we assert on, perhaps taken immediately before the superblock creation. You might need to separate the timestamp check from the main assert_inode_stat! macro.

We should update these tests:

https://github.com/awslabs/s3-file-connector/blob/b23b39d5b2e619116f58e492f6f023da11dbfc97/s3-file-connector/src/inode.rs#L766-L872

s3-file-connector/src/inode.rs Outdated Show resolved Hide resolved
s3-file-connector/src/inode.rs Outdated Show resolved Hide resolved
@dannycjones dannycjones added this to the alpha milestone Feb 21, 2023
@dannycjones dannycjones linked an issue Feb 21, 2023 that may be closed by this pull request
Signed-off-by: Ankit Saurabh <sauraank@amazon.co.uk>
Copy link
Contributor

@dannycjones dannycjones left a comment

Choose a reason for hiding this comment

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

I think this looks good!

It would be nice to add some tests to test_readdir and then ready to merge I think.

https://github.com/awslabs/s3-file-connector/blob/66b01ccbe1d01251db832212bac96a59c7185d1c/s3-file-connector/src/inode.rs#L859-L912

s3-file-connector/src/inode.rs Outdated Show resolved Hide resolved
Signed-off-by: Ankit Saurabh <sauraank@amazon.co.uk>
Copy link
Contributor

@dannycjones dannycjones left a comment

Choose a reason for hiding this comment

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

Almost ready to merge, I think we just need to expand a bit on the readdir test.

s3-file-connector/src/inode.rs Outdated Show resolved Hide resolved
Signed-off-by: Ankit Saurabh <sauraank@amazon.co.uk>
s3-file-connector/src/inode.rs Outdated Show resolved Hide resolved
Signed-off-by: Ankit Saurabh <sauraank@amazon.co.uk>
Copy link
Contributor

@dannycjones dannycjones left a comment

Choose a reason for hiding this comment

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

One small comment and then I think this is ready to merge.

s3-file-connector/src/inode.rs Outdated Show resolved Hide resolved
Signed-off-by: Ankit Saurabh <sauraank@amazon.co.uk>
Copy link
Contributor

@dannycjones dannycjones left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for improving the existing tests that missed Readdir for file mtime as well.

@dannycjones dannycjones added the enhancement New feature or request label Feb 27, 2023
@dannycjones dannycjones enabled auto-merge (squash) February 27, 2023 14:16
@dannycjones dannycjones merged commit 9c2b821 into main Feb 27, 2023
@dannycjones dannycjones deleted the mtimeDirectory branch February 27, 2023 14:31
@dannycjones
Copy link
Contributor

FYI there was a flaky test run unrelated to this change, I opened #116.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement directory atime,ctime,mtime
2 participants