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

Add GetTemperature on existing files #9498

Closed
wants to merge 1 commit into from

Conversation

pdillinger
Copy link
Contributor

Summary: For tiered storage

Test Plan: Just API placeholders for now

Summary: For tiered storage

Test Plan: Just API placeholders for now
Copy link
Contributor

@siying siying left a comment

Choose a reason for hiding this comment

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

It feels like a good function call for validation purpose regardless whether we build tiered storage migration using it.

@facebook-github-bot
Copy link
Contributor

@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@pdillinger
Copy link
Contributor Author

I'm thinking about revising or extending this. We probably want to be able to find out the tier without opening the file, or at least have that option. We have some options:

  • Add FileSystem::GetTemperature(const std::string& path, ...)
  • Add temperature to FileAttributes, though many FileSystems rely on the default implementation so we would likely want FileSystem::GetTemperature as well. This API might allow some implementations to speed up fetching the info for many files in the same directory, or it might upset/confuse existing implementations populating FileAttributes.

And if we have these, do we need the versions that tell you about an open file?

  • Especially on systems where a file descriptor can refer to an old file no longer associated with the name, the versions on an open file ensure the temperature reflects the tier info of the open file. This is not typically something we worry about but is a hypothetical concern that could arise.
  • The versions on an open file are likely more efficient if opening the file anyway, because it can avoid a second seek to the file by name.

@mrambacher
Copy link
Contributor

I'm thinking about revising or extending this. We probably want to be able to find out the tier without opening the file, or at least have that option. We have some options:

  • Add FileSystem::GetTemperature(const std::string& path, ...)
  • Add temperature to FileAttributes, though many FileSystems rely on the default implementation so we would likely want FileSystem::GetTemperature as well. This API might allow some implementations to speed up fetching the info for many files in the same directory, or it might upset/confuse existing implementations populating FileAttributes.

And if we have these, do we need the versions that tell you about an open file?

  • Especially on systems where a file descriptor can refer to an old file no longer associated with the name, the versions on an open file ensure the temperature reflects the tier info of the open file. This is not typically something we worry about but is a hypothetical concern that could arise.
  • The versions on an open file are likely more efficient if opening the file anyway, because it can avoid a second seek to the file by name.

I was going to suggest adding a FileSystem::GetFileAttributes() method that returns the attributes of a file (size, modification time, temperature). Currently, you can only get the attributes of all files in a directory. I think this makes more sense than being able to get the Temperature from the various FSxxFile classes.

@pdillinger
Copy link
Contributor Author

I'm not ruling out adding more ways to access the info in the future, but I think these functions are useful because they should suffice for our near-term needs and likely be simplest to implement within the FileSystem. And longer term (as mentioned above) GetTemperature() on an open file is likely more efficient if opening the file anyway, because it can avoid a second seek to the file by name, or potentially a directory listing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants