Skip to content
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: allow the specification of a write window for retention policies #25517

Merged
merged 12 commits into from
Nov 15, 2024

Conversation

davidby-influx
Copy link
Contributor

@davidby-influx davidby-influx commented Nov 4, 2024

Add FutureWriteLimit and PastWriteLimit to
retention policies. Points which are outside of
now() + FutureWriteLimit
or
now() - PastWriteLimit
will be rejected with a PartialWriteError.

Closes #25424

@gwossum
Copy link
Member

gwossum commented Nov 5, 2024

@davidby-influx It looks like if an RP has a future limit or a past limit that there is no way to remove it completely, just alter the duration. At the very least, I don't seem to see a test for removing a write limit from an RP.

@davidby-influx
Copy link
Contributor Author

@gwossum - You are correct - there is not explicit removal. Write limits that are set to zero are ignored/not printed/removed. I will add a test for that.

Copy link

@devanbenz devanbenz left a comment

Choose a reason for hiding this comment

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

Looks go to me. No major changes. Just a few comments.

coordinator/points_writer.go Outdated Show resolved Hide resolved
coordinator/points_writer.go Outdated Show resolved Hide resolved
coordinator/points_writer.go Show resolved Hide resolved
devanbenz
devanbenz previously approved these changes Nov 15, 2024
Copy link

@devanbenz devanbenz left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 117 to 119
if (rp != nil) &&
(((rp.FutureWriteLimit > 0) && p.Time().After(time.Now().Add(rp.FutureWriteLimit))) ||
((rp.PastWriteLimit > 0) && p.Time().Before(time.Now().Add(-rp.PastWriteLimit)))) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we shouldn't move this check into PointsWriter.MapShards. Having the check in here could allow us to create shards outside of the write window before we drop the points here. Moving the check into PointsWriter.MapShards would prevent the creation of shards outside the write windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.

Comment on lines +126 to +127
(((rp.FutureWriteLimit > 0) && p.Time().After(time.Now().Add(rp.FutureWriteLimit))) ||
((rp.PastWriteLimit > 0) && p.Time().Before(time.Now().Add(-rp.PastWriteLimit)))) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion for future optimization: Create a writeWindow object that caches the value of time.Now().Add(rp.FutureWriteLimit) and time.Now().Add(-rp.PastWriteLimit) to avoid calculating them twice for every point in request.

Copy link
Member

@gwossum gwossum left a comment

Choose a reason for hiding this comment

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

LGTM. There's a potential optimization to avoid recalculating the write window twice for every point, but works as-is.

@davidby-influx davidby-influx merged commit 07c261a into master-1.x Nov 15, 2024
9 checks passed
@davidby-influx davidby-influx deleted the DSB_write_limit branch November 15, 2024 21:30
@gunnaraasen
Copy link
Member

In the release notes, we should mention the addition of InfluxQL reserved tokens for FUTURE and PAST, which may end up breaking a queries where future or past are used as identifiers and are not double quoted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow the specification of a write window for retention policies
4 participants