Conversation
5ee8193 to
b0d33f9
Compare
b0d33f9 to
7d9cf2a
Compare
stuartcarnie
left a comment
There was a problem hiding this comment.
Does what it says on the box – nice work!
Was a good study for me too, as I'll eventually have to add something similar for adding API to perform / update the hard delete date.
| // Return the oldest allowable timestamp for the given table according to the | ||
| // currently-available set of retention policies. This is returned as a number of nanoseconds | ||
| // since the Unix Epoch. | ||
| pub fn get_retention_period_cutoff_ts_nanos(&self, db_id: &DbId, _: &TableId) -> Option<i64> { |
There was a problem hiding this comment.
Note
Just my musing, not something that needs to be changed for this PR
I have been thinking that adding a specific UnixTimestamp would be beneficial. We handle nanosecond UNIX epoch timestamps regularly, but an i64 doesn't carry any semantic meaning. Passing around UnixTimestamp would be clearer. I found a crate, unix-ts, but that stores the value as an i128. We could consider adding our own for i64.
There was a problem hiding this comment.
I like the sound of that as a general repo-wide refactor, but for this purpose I just stuck with the type used to represent timestamps wherever these values will be used.
There was a problem hiding this comment.
Same – definitely should be a deliberate PR. Start with developing the type with a good API, and then a separate PR to do the repo-wide refactor.
| retention_period: snap | ||
| .retention_period | ||
| .map(Snapshot::from_snapshot) | ||
| .unwrap_or(RetentionPeriod::Indefinite), |
There was a problem hiding this comment.
✅ default to Indefinite is snap.retention_period is None
hiltontj
left a comment
There was a problem hiding this comment.
I had a couple of comments (none blocking).
| let now = self.time_provider.now().timestamp_nanos(); | ||
| Some(now - retention_period as i64) |
There was a problem hiding this comment.
FWIW there is a Time::checked_sub API that you could use here to directly subtract the retention period Duration from self.time_provider.now().
| db_name: &str, | ||
| duration: Duration, | ||
| ) -> Result<OrderedCatalogBatch> { | ||
| info!("create new retention policy"); |
There was a problem hiding this comment.
It would be useful to log the db_name and duration provided.
| &self, | ||
| db_name: &str, | ||
| ) -> Result<OrderedCatalogBatch> { | ||
| info!("delete retention policy"); |
There was a problem hiding this comment.
It would be useful to log the db_name
* fix: Automatic intermediate directory cleanup for file object store (#26480) Removes empty intermediate directories when a key is removed from local file system object storage, which matches cloud-based providers. * feat: add retention period to catalog (#26479) * feat: add retention period to catalog * fix: handle humantime parsing error properly * refactor: use new iox_http_util types --------- Co-authored-by: Michael Gattozzi <mgattozzi@influxdata.com> * chore: address a couple post-merge PR comments (#26489) * feat: add concurrency limit for WAL replay (#26483) WAL replay currently loads _all_ WAL files concurrently running into OOM. This commit adds a CLI parameter `--wal-replay-concurrency-limit` that would allow the user to set a lower limit and run WAL replay again. closes: #26481 --------- Co-authored-by: Stuart Carnie <stuart.carnie@gmail.com> Co-authored-by: Michael Gattozzi <mgattozzi@influxdata.com> Co-authored-by: praveen-influx <pkumar@influxdata.com>
This is the first step in implementation for #26169
This adds catalog-level and http-level support for setting retention periods on databases. It does not implement retention period handling in management of query buffers or object store contents. The intent here is just to support the catalog types as a base for subsequent work.
The major change to take note of here is that we are adding a new
retention_periodfield to databases which is represented by aRetentionPeriodenum which can either be:RetentionPeriod::IndefiniteRetentionPeriod::Duration(t)