Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

Retention ready timestamp #1178

Merged
merged 4 commits into from
Dec 13, 2018
Merged

Retention ready timestamp #1178

merged 4 commits into from
Dec 13, 2018

Conversation

Dieterbe
Copy link
Contributor

@Dieterbe Dieterbe commented Dec 13, 2018

allow retention 'ready' to be a unix timestamp.
this helps in situations where an archive for example has TTL of 2 months, but only 1 month worth of data.
previously we could only control whether the archive was used (for all of the 2 months window) or not.
now we can control to only use it for requests that need data more recent than 1 month.

@Dieterbe Dieterbe force-pushed the retention-ready-timestamp branch from 1578fbf to 226aebe Compare December 13, 2018 11:47
@Dieterbe Dieterbe changed the title WIP: Retention ready timestamp Retention ready timestamp Dec 13, 2018
@Dieterbe Dieterbe requested a review from woodsaj December 13, 2018 11:47
@Dieterbe Dieterbe force-pushed the retention-ready-timestamp branch from 226aebe to 8567fed Compare December 13, 2018 12:09
@Dieterbe Dieterbe force-pushed the retention-ready-timestamp branch from 8567fed to 4585ff7 Compare December 13, 2018 12:26
retention.Ready, err = strconv.ParseBool(parts[4])
if err != nil {
return nil, err
// 0 (default) is effectively the same as 'true'
Copy link
Member

Choose a reason for hiding this comment

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

Need to clarify that the ready field used to be true|false. Otherwise, this comment won't make any sense when read in 6months time.

docs/config.md Outdated
# so you rather query other archives, even if they don't have the retention to serve your queries
# ready: whether, or as of what data timestamp, the archive is ready for querying.
# This is useful if you recently introduced a new archive, but it's still being populated, so doesn't have the data metrictank might otherwise think there is
# It supports to syntaxes:
Copy link
Member

Choose a reason for hiding this comment

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

two syntaxes

Copy link
Member

@woodsaj woodsaj left a comment

Choose a reason for hiding this comment

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

minor suggestion for code comments, otherwise LGTM

@Dieterbe Dieterbe merged commit 60eb98e into master Dec 13, 2018
@Dieterbe Dieterbe deleted the retention-ready-timestamp branch March 27, 2019 21:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants