-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add runtime config options for list_files_cache_limit and list_files_cache_ttl
#19108
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
base: main
Are you sure you want to change the base?
Conversation
list_files_cache_limit and list_files_cache_ttl
| SELECT name FROM information_schema.df_settings WHERE name LIKE 'datafusion.runtime.%' ORDER BY name | ||
| ---- | ||
| datafusion.runtime.list_files_cache_limit | ||
| datafusion.runtime.list_files_cache_ttl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The values for these will not update after running SET, unless the runtime has been configured with a ListFilesCache (it's None by default, so there is nothing to update)
|
Hi @BlakeOrth @alamb, this is ready for review |
BlakeOrth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This overall looks very nice to me, thanks! Perhaps I'm missing it, but is there anyway to set the TTL back to infinity (None) after it's been set to Some(Duration)?
| } | ||
| } | ||
|
|
||
| fn parse_duration(duration: &str) -> Result<Duration> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this method error if the input duration evaluates to 0s?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that would make sense eaff9b5
|
@BlakeOrth Yep, you can run |
BlakeOrth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My approval doesn't have any real power here, but this all looks good to me. The CI failure seems like it's probably unrelated. I can't imagine this work having any effect on benchmarks.
|
Thanks @delamarch3 and @BlakeOrth -- I'll try and check this out soon |
Which issue does this PR close?
Rationale for this change
Make the list file cache memory limit and TTL configurable via runtime config.
What changes are included in this PR?
SETandRESETlist_files_cache_limitandlist_files_cache_ttllist_files_cache_ttlwill expect the duration to look like either1m30sor30(I'm wondering if it would be simpler for it to just accept a single unit?)update_cache_ttl()to theListFilesCachetrait so we can update it fromRuntimeEnvBuilder::build()Are these changes tested?
Yes
Are there any user-facing changes?
update_cache_ttl()has been added to theListFilesCachetrait