feat: loosen 72 hour query/write restriction #25890
Conversation
mgattozzi
requested review from
pauldix,
waynr,
hiltontj,
jacksonrnewhouse and
praveen-influx
influxdb3/src/commands/serve.rs
Outdated
| /// query performance will likely suffer as a result. It would be better to specify smaller time | ||
| /// ranges if possible in a query | ||
| #[clap(long = "file-limit", env = "INFLUXDB3_FILE_LIMIT", action)] | ||
| pub file_limit: Option<usize>, |
There was a problem hiding this comment.
Just a minor thing, setting 432 here directly as a default here could've worked?
There was a problem hiding this comment.
I'd still have to pass it in for testing purposes so I kept it as an Option so that there would only be one place that the default would need to be defined!
There was a problem hiding this comment.
Can we rename to query-file-limit? I think it's better to be more descriptive about what this file limit is about.
influxdb3_write/src/lib.rs
Outdated
| @@ -570,6 +567,7 @@ impl BufferFilter { | |||
| < analysis.boundaries.len()) | |||
| .then_some(analysis.boundaries.remove(time_col_index)) | |||
| { | |||
| dbg!(&interval); | |||
There was a problem hiding this comment.
Should these dbg!s be left in?
There was a problem hiding this comment.
No I thought I had taken them out!
influxdb3_write/src/lib.rs
Outdated
| @@ -578,6 +576,7 @@ impl BufferFilter { | |||
| } else { | |||
| time_interval.replace(interval); | |||
| } | |||
| dbg!(&time_interval); | |||
| let db_name = "test_db"; | ||
| // perform writes over time to generate WAL files and some snapshots | ||
| // the time provider is bumped to trick the system into persisting files: | ||
| for i in 0..1298 { |
There was a problem hiding this comment.
I see in the comment that the time provider is bumped to trick the system into persisting files, but just so I understand correctly -- is that because the system is configured at some level to persist to the object store at some interval, ie 30 seconds or so?
There was a problem hiding this comment.
Yes exactly! Normally this would be by default I think every 10 minutes or so.
There was a problem hiding this comment.
Snapshot happens based on the number of WAL files. Incrementing the time makes it so that the writes fall into different gen1 blocks of time, each of which will result in a single parquet file.
mgattozzi
force-pushed
the
mgattozzi/remove-72-hour-limit
branch
from
7c3317e to
218efa0
Compare
influxdb3/src/commands/serve.rs
Outdated
| /// query performance will likely suffer as a result. It would be better to specify smaller time | ||
| /// ranges if possible in a query | ||
| #[clap(long = "file-limit", env = "INFLUXDB3_FILE_LIMIT", action)] | ||
| pub file_limit: Option<usize>, |
There was a problem hiding this comment.
Can we rename to query-file-limit? I think it's better to be more descriptive about what this file limit is about.
| let db_name = "test_db"; | ||
| // perform writes over time to generate WAL files and some snapshots | ||
| // the time provider is bumped to trick the system into persisting files: | ||
| for i in 0..1298 { |
There was a problem hiding this comment.
Snapshot happens based on the number of WAL files. Incrementing the time makes it so that the writes fall into different gen1 blocks of time, each of which will result in a single parquet file.
This commit does a few key things: - Removes the 72 hour query and write restrictions in Core - Limits the queries to a default number of parquet files. We chose 432 as this is about 72 hours using default settings for the gen1 timeblock - The file limit can be increased, but the help text and error message when exceeded note that query performance will likely be degraded as a result. - We warn users to use smaller time ranges if possible if they hit this query error With this we eliminate the hard restriction we have in place, but instead create a soft one that users can choose to take the performance hit with. If they can't take that hit then it's recomended that they upgrade to Enterprise which has the compactor built in to make performant historical queries.
mgattozzi
force-pushed
the
mgattozzi/remove-72-hour-limit
branch
from
218efa0 to
7a9ade9
Compare
|
Updated wording and moved us to using --query-file-limit |
This commit does a few key things:
With this we eliminate the hard restriction we have in place, but instead create a soft one that users can choose to take the performance hit with. If they can't take that hit then it's recommended that they upgrade to Enterprise which has the compactor built in to make performant historical queries.