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

Write file temperature information to manifest #8284

Closed
wants to merge 4 commits into from

Conversation

siying
Copy link
Contributor

@siying siying commented May 10, 2021

Summary:
As a part of tiered storage, writing tempeature information to manifest is needed so that after DB recovery, RocksDB still has the tiering information, to implement some further necessary functionalities.

Also fix some issues in simulated hybrid FS.

Test Plan: Add a new unit test to validate that the information is indeed written and read back.

@facebook-github-bot
Copy link
Contributor

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

kCold,
kTotal,
kUnknown = kTotal,
kUnknown = 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we give a larger range between kHot and kCold for future extension not just 1 to 3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm... I don't know. I can leave some room just in case.

Copy link
Contributor

@zhichao-cao zhichao-cao left a comment

Choose a reason for hiding this comment

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

Should we also give a temperature to the flushed file? I see in 8222, we only give the temperature to the bottommost files.

@siying
Copy link
Contributor Author

siying commented May 17, 2021

Should we also give a temperature to the flushed file? I see in 8222, we only give the temperature to the bottommost files.

That's a good question. Maybe we can delay the decision a little bit? The downside of tagging a file hot is that we will need to put a field in the manifest file. Doing it for a unfinished feature feels scaring to me. Maybe we can first only tag cold files, which is necessary for development and benchmarking, and decide whether we need to tag hot files too in the future?

@zhichao-cao
Copy link
Contributor

Should we also give a temperature to the flushed file? I see in 8222, we only give the temperature to the bottommost files.

That's a good question. Maybe we can delay the decision a little bit? The downside of tagging a file hot is that we will need to put a field in the manifest file. Doing it for a unfinished feature feels scaring to me. Maybe we can first only tag cold files, which is necessary for development and benchmarking, and decide whether we need to tag hot files too in the future?

Yeah, it make sense.

Copy link
Contributor

@zhichao-cao zhichao-cao 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!

@@ -87,7 +87,8 @@ enum NewFileCustomTag : uint32_t {
kCustomTagNonSafeIgnoreMask = 1 << 6,

// Forward incompatible (aka unignorable) fields
kPathId,
kPathId = (1 << 6) | 1,
kTemperature = (1 << 6) | 2,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm rethinking about it and moving it to forward compatible field.

Summary:
As a part of tiered storage, writing tempeature information to manifest is needed so that after DB recovery, RocksDB still has the tiering information, to implement some further necessary functionalities.

Also fix some issues in simulated hybrid FS.

Test Plan: Add a new unit test to validate that the information is indeed written and read back.
@facebook-github-bot
Copy link
Contributor

@siying has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@siying has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 0ed8cb6.

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.

3 participants