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

Auto-expire old items from FIFO cache #5148

Merged
merged 3 commits into from
Jan 31, 2022
Merged

Conversation

chaudum
Copy link
Contributor

@chaudum chaudum commented Jan 14, 2022

What this PR does / why we need it:

Addresses the problem of the FIFO cache where items become stale due to the validity setting but are never purged, as described in #4921

Which issue(s) this PR fixes:
Fixes #4921

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated
  • Add an entry in the CHANGELOG.md about the changes.

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

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

LGTM

You should add a changelog entry.

@chaudum chaudum force-pushed the chaudum/remove-validity-from-fifo branch from 3fa4ff5 to cf120f0 Compare January 17, 2022 14:36
@chaudum
Copy link
Contributor Author

chaudum commented Jan 17, 2022

@cyriltovena I've moved the ticker creation into the goroutine. Do you want to have another look?

@cyriltovena
Copy link
Contributor

@cyriltovena I've moved the ticker creation into the goroutine. Do you want to have another look?

Let's wait for @KMiller-Grafana and I'll merge it.

Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

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

Nice work! LGTM modulo pending suggestions from Cyril.

@slim-bean
Copy link
Collaborator

Very nice! thanks for adding this @chaudum!

@chaudum chaudum force-pushed the chaudum/remove-validity-from-fifo branch from b1690b5 to f13e556 Compare January 18, 2022 16:29
@chaudum
Copy link
Contributor Author

chaudum commented Jan 20, 2022

Hey @KMiller-Grafana, could I get a review of the (small) docs changes I did on this PR so we can merge it?

@chaudum chaudum force-pushed the chaudum/remove-validity-from-fifo branch from f13e556 to 7f00665 Compare January 24, 2022 07:26
Copy link
Contributor

@KMiller-Grafana KMiller-Grafana left a comment

Choose a reason for hiding this comment

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

Small change suggestions.

docs/sources/configuration/_index.md Outdated Show resolved Hide resolved
docs/sources/configuration/_index.md Outdated Show resolved Hide resolved
chaudum and others added 3 commits January 29, 2022 13:23
We can remove the validity setting of 1h in 2.4 for the
chunks cache because it doesn't reduce memory usage and instead leads to
valid chunks being ignore when querying the cache. Chunks are immutable
so it doesn't make sense to not return them if they are present in the
cache.

Closes #4922

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
As described in #4921 this PR adds a periodic task that prunes expired
items from the FIFO cache to free up memory.

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
Co-authored-by: Karen Miller <84039272+KMiller-Grafana@users.noreply.github.com>
@owen-d owen-d merged commit 57b384d into main Jan 31, 2022
@owen-d owen-d deleted the chaudum/remove-validity-from-fifo branch January 31, 2022 15:38
lxwzy added a commit to lxwzy/loki that referenced this pull request Jul 9, 2022
As grafana#5148 changed, the deprecated is fifocache.duration, not fifocache.interval.
lxwzy added a commit to lxwzy/loki that referenced this pull request Nov 7, 2022
As grafana#5148 changed, the deprecated is fifocache.duration, not fifocache.interval.
MichelHollands pushed a commit that referenced this pull request Nov 7, 2022
…ed (#7609)

**What this PR does / why we need it**:
I configured fifo cache with `validity` option (which is
[deprecated](https://github.com/grafana/loki/blob/b92f113cb0961b628b643f4f3f9fddc1c18ed921/docs/sources/configuration/_index.md?plain=1#L2004)
in v2.6.0, `fifocache.duration` as an flag) and loki print that:

```
level=warn ts=2022-07-08T17:01:30.134251284Z caller=fifo_cache.go:125 msg="running with DEPRECATED flag fifocache.interval, use fifocache.ttl instead" cache=frontend.fifocache
```

However, there is no flag `fifocache.interval`.
As #5148 changed, the deprecated flag should be `fifocache.duration`,
not `fifocache.interval`.
changhyuni pushed a commit to changhyuni/loki that referenced this pull request Nov 8, 2022
…ed (grafana#7609)

**What this PR does / why we need it**:
I configured fifo cache with `validity` option (which is
[deprecated](https://github.com/grafana/loki/blob/b92f113cb0961b628b643f4f3f9fddc1c18ed921/docs/sources/configuration/_index.md?plain=1#L2004)
in v2.6.0, `fifocache.duration` as an flag) and loki print that:

```
level=warn ts=2022-07-08T17:01:30.134251284Z caller=fifo_cache.go:125 msg="running with DEPRECATED flag fifocache.interval, use fifocache.ttl instead" cache=frontend.fifocache
```

However, there is no flag `fifocache.interval`.
As grafana#5148 changed, the deprecated flag should be `fifocache.duration`,
not `fifocache.interval`.
Abuelodelanada pushed a commit to canonical/loki that referenced this pull request Dec 1, 2022
…ed (grafana#7609)

**What this PR does / why we need it**:
I configured fifo cache with `validity` option (which is
[deprecated](https://github.com/grafana/loki/blob/b92f113cb0961b628b643f4f3f9fddc1c18ed921/docs/sources/configuration/_index.md?plain=1#L2004)
in v2.6.0, `fifocache.duration` as an flag) and loki print that:

```
level=warn ts=2022-07-08T17:01:30.134251284Z caller=fifo_cache.go:125 msg="running with DEPRECATED flag fifocache.interval, use fifocache.ttl instead" cache=frontend.fifocache
```

However, there is no flag `fifocache.interval`.
As grafana#5148 changed, the deprecated flag should be `fifocache.duration`,
not `fifocache.interval`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FIFO Cache validity behavior and lack of auto expiration
6 participants