Skip to content

docs: Add AsyncDataCache and SSD cache documentation#11429

Closed
minhancao wants to merge 1 commit intofacebookincubator:mainfrom
minhancao:ssd_cache_doc
Closed

docs: Add AsyncDataCache and SSD cache documentation#11429
minhancao wants to merge 1 commit intofacebookincubator:mainfrom
minhancao:ssd_cache_doc

Conversation

@minhancao
Copy link
Collaborator

@minhancao minhancao commented Nov 5, 2024 •
edited
Loading

Add AsyncDataCache and SSD cache documentation

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 5, 2024
@netlify
Copy link

netlify bot commented Nov 5, 2024 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 2078073
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/67e1d0cc730af900087a2ce5

@minhancao minhancao changed the title doc: Adding async data cache and ssd cache documentation doc: Adding async data cache configs and ssd cache documentation Nov 5, 2024
@minhancao minhancao marked this pull request as ready for review November 6, 2024 22:43
@minhancao
Copy link
Collaborator Author

minhancao commented Nov 12, 2024

@majetideepak Can you review this PR when you can? Thanks!

czentgr
czentgr reviewed Nov 15, 2024
View reviewed changes
Copy link
Collaborator

@czentgr czentgr left a comment

Choose a reason for hiding this comment

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

Thanks for this. Multiple comments.

@minhancao minhancao changed the title doc: Adding async data cache configs and ssd cache documentation docs: Adding async data cache configs and ssd cache documentation Nov 19, 2024
@ethanyzhang
Copy link
Collaborator

ethanyzhang commented Jan 7, 2025

@czentgr to re-review

czentgr
czentgr reviewed Feb 11, 2025
View reviewed changes
velox/docs/develop/cache.rst Outdated
- The size of the SSD.
* - async-cache-ssd-path
- string
- /mnt/flash/async_cache.
Copy link
Collaborator

@czentgr czentgr Feb 11, 2025

Choose a reason for hiding this comment

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

Does this have a . at the end of the path? I think this is a typo.

Copy link
Collaborator Author

@minhancao minhancao Feb 19, 2025

Choose a reason for hiding this comment

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

@czentgr It is set to default to have the . at the end of the path in https://github.com/prestodb/presto/blob/master/presto-native-execution/presto_cpp/main/common/Configs.cpp#L184

czentgr
czentgr reviewed Feb 19, 2025
View reviewed changes
@minhancao minhancao force-pushed the ssd_cache_doc branch 2 times, most recently from c83c939 to 1b01d4e Compare February 19, 2025 23:19
majetideepak
majetideepak reviewed Feb 20, 2025
View reviewed changes
velox/docs/develop/cache.rst Outdated
disk_names=( $(sudo lsblk -d -o NAME | tail -n +2) )
for disk in "${disk_names[@]}"; do
echo "Checking disk: $disk"
# If the disk is an Amazon EC2 NVMe Instance Storage volume, then install btrfs onto that disk
Copy link
Collaborator

@majetideepak majetideepak Feb 20, 2025

Choose a reason for hiding this comment

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

This is AWS EC2 specific. I am not convinced this should be part of the main documentation since it can easily explode for various setups.
We need to find a home for such operational details.
Orri said they are writing a document on SSD cache design
I would suggest we limit this PR to config updates and move the Operational part to another PR.

Copy link
Collaborator Author

@minhancao minhancao Feb 21, 2025

Choose a reason for hiding this comment

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

@majetideepak Ok will remove the whole "Setup with btrfs filesystem" section and move this to another doc PR. Where do you think we should document the Operational part?

Copy link
Collaborator

@majetideepak majetideepak Feb 21, 2025

Choose a reason for hiding this comment

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

We can check with Meta on a good location for operational details in the other PR.

@majetideepak majetideepak changed the title docs: Adding async data cache configs and ssd cache documentation docs: Adding async data cache and ssd cache configs Feb 21, 2025
majetideepak
majetideepak reviewed Feb 21, 2025
View reviewed changes
@minhancao
Copy link
Collaborator Author

minhancao commented Feb 24, 2025 •
edited
Loading

@majetideepak It looks like prestodb/presto#24372 that was merged Jan 16, 2025 to Prestissimo removed the query-data-cache-enabled-default config. Should cache.no_retention config be removed as well?

From prestodb/presto#24372:

== RELEASE NOTES ==

General Changes
* Remove ``query-data-cache-enabled-default`` configuration property, which is no longer needed as per-split fine-grained cache control has been introduced. :pr:`24372 `

@minhancao minhancao mentioned this pull request Feb 24, 2025
Merged
6 tasks
minhancao
minhancao commented Feb 24, 2025
View reviewed changes
velox/docs/develop/cache.rst Outdated

Configuration Properties
------------------------
See `Configuration Properties <../configs.rst>`_ for AsyncDataCache related configuration properties.
Copy link
Collaborator Author

@minhancao minhancao Feb 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@majetideepak I have created a new PR in Prestissimo to have all the configuration properties of AsyncDataCache and SSD cache listed there since those configs are defined in Prestissimo codebase. Once that PR is in, I will link it to here. What do you think of this approach?

prestodb/presto#24623

minhancao added a commit to minhancao/velox that referenced this pull request Mar 24, 2025
@minhancao
docs: Add AsyncDataCache and SSD cache documentation (facebookincubat…
a8b9e5d 
…or#11429)
@minhancao minhancao changed the title docs: Adding async data cache and ssd cache configs docs: Add AsyncDataCache and SSD cache documentation Mar 24, 2025
czentgr
czentgr reviewed Mar 26, 2025
View reviewed changes
velox/docs/develop/cache.rst
@@ -0,0 +1,39 @@
=====================================
Copy link
Collaborator

@czentgr czentgr Mar 26, 2025

Choose a reason for hiding this comment

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

I'm now doubting the need for this PR. Do we need this in addition to what we have in the PrestoC++ doc?
This content looks more like PrestoC++ documentation content with the references to the Velox doc.

All the content here sounds more like at the PrestoC++ level rather than Velox. The Velox documentation would rather go into detail as to how it works rather than talk about the operationalization and some vague statements.

Copy link
Collaborator Author

@minhancao minhancao Mar 31, 2025

Choose a reason for hiding this comment

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

@czentgr I agree, I will close this PR since the cache configs PR already was merged for Prestissimo.

@minhancao minhancao closed this Mar 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@majetideepak majetideepak majetideepak left review comments

@xiaoxmeng xiaoxmeng Awaiting requested review from xiaoxmeng

+1 more reviewer

@czentgr czentgr czentgr left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@minhancao @ethanyzhang @majetideepak @czentgr @facebook-github-bot