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.

Sign up for GitHub

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

Jump to bottom

Add real values for file atime, ctime, and mtime #48

Merged
merged 3 commits into from Jan 19, 2023
Merged

Add real values for file atime, ctime, and mtime #48

merged 3 commits into from Jan 19, 2023

Conversation

dannycjones
Copy link
Contributor

This PR plumbs in atime, ctime, and mtime through the inode module, instead of defaults in the fs module.

It uses the last modified time of the object for all three values. It also disables atime updates using the FUSE option, as the connector does not support atime updates.

Note: it does not implement nor plumb in crtime or creation time, which appears to be a macOS option.

Feedback on testing very welcome also.

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

@dannycjones dannycjones self-assigned this Jan 16, 2023
@dannycjones dannycjones linked an issue Jan 16, 2023 that may be closed by this pull request
Implement file mtime,ctime,atime #35
Closed
@dannycjones dannycjones changed the title Add real values for atime, ctime, and mtime Add real values for file atime, ctime, and mtime Jan 16, 2023
@dannycjones
Copy link
Contributor Author

Also note this feedback from Andy in the issue:

We've talked about the idea of having different modes/personalities for the connector in the future, so it might be worth thinking about how to structure this in a way that helps support that later.

@dannycjones dannycjones marked this pull request as draft January 16, 2023 18:17
@dannycjones
Copy link
Contributor Author

dannycjones commented Jan 16, 2023 •
edited

Leaving in draft, as it is probably the right time to tackle directory stats too. I'd still like feedback as the directory stats should be straightforward following similar changes.

@dannycjones dannycjones added this to the alpha milestone Jan 16, 2023
jamesbornholt
jamesbornholt reviewed Jan 16, 2023
View reviewed changes
Copy link
Member

@jamesbornholt jamesbornholt left a comment

Choose a reason for hiding this comment

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

This approach looks right to me! Just a few suggestions.

s3-file-connector/Cargo.toml Outdated Show resolved Hide resolved
s3-file-connector/src/inode.rs Outdated Show resolved Hide resolved
s3-file-connector/src/inode.rs Outdated Show resolved Hide resolved
s3-file-connector/src/inode.rs Outdated Show resolved Hide resolved
s3-client/src/mock_client.rs Outdated Show resolved Hide resolved
s3-file-connector/src/inode.rs Outdated Show resolved Hide resolved
s3-file-connector/src/inode.rs Outdated Show resolved Hide resolved
monthonk
monthonk reviewed Jan 17, 2023
View reviewed changes
Copy link
Contributor

@monthonk monthonk 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! Left a few comments.

s3-file-connector/src/inode.rs Outdated Show resolved Hide resolved
s3-file-connector/src/inode.rs Outdated Show resolved Hide resolved
@dannycjones dannycjones marked this pull request as ready for review January 18, 2023 17:24
@dannycjones dannycjones mentioned this pull request Jan 18, 2023
Closed
jamesbornholt
jamesbornholt requested changes Jan 18, 2023
View reviewed changes
Copy link
Member

@jamesbornholt jamesbornholt left a comment

Choose a reason for hiding this comment

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

LGTM, just one nit.

s3-file-connector/src/inode.rs Outdated Show resolved Hide resolved
@dannycjones dannycjones merged commit 8a832dd into main Jan 19, 2023
@dannycjones dannycjones deleted the inode-stat-time branch January 19, 2023 10:43
@dannycjones dannycjones mentioned this pull request Feb 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamesbornholt jamesbornholt jamesbornholt approved these changes

@monthonk monthonk monthonk approved these changes

Assignees

@dannycjones dannycjones

Labels
None yet
Projects
None yet
Milestone
Alpha (Public Release)
Development

Successfully merging this pull request may close these issues.

Implement file mtime,ctime,atime
3 participants
@dannycjones @jamesbornholt @monthonk