-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
feat: enable parquet cache config through CLI #25415
Conversation
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.
Just a couple of small comments.
influxdb3/src/commands/serve.rs
Outdated
@@ -216,6 +217,67 @@ pub struct Config { | |||
/// for any hosts that share the same object store configuration, i.e., the same bucket. | |||
#[clap(long = "host-id", env = "INFLUXDB3_HOST_IDENTIFIER_PREFIX", action)] | |||
pub host_identifier_prefix: String, | |||
|
|||
/// The size of the in-memory Parquet cache in bytes. |
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.
Would it be better/more user friendly to have this value in MB?
influxdb3/src/commands/serve.rs
Outdated
#[clap( | ||
long = "parquet-mem-cache-prune-interval", | ||
env = "INFLUXDB3_PARQUET_MEM_CACHE_PRUNE_INTERVAL", | ||
default_value = "10ms", |
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.
Does this just grab a read lock? Seems like we don't need to check this so frequently by default, once a second seems more than enough?
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, only if it needs to prune would lock the inner map, but I agree, this is a little over-zealous.
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.
LGTM - apart from the points Paul's raised
aa6e3be
to
ed00ced
Compare
Closes #25395
This adds the following options to the
influxdb3 serve
CLI:With respect to the original issue, I decided to add the prune percent and interval asa CLI options, since if we or the perf team end up needing them there for whatever reason, it would be a pain if they were omitted.