Issue-5320 clean expired cache entries periodically #6392

rudrakhp · 2023-11-07T15:21:12Z

Why the changes in this PR are needed?

Regular clean up of cache entries that have expired for a more efficient use of memory.
Removing stale entries regularly will make space for newer entries instead of forcefully evicting entries in a FIFO fashion which happens today. Sometimes entries in front might be long living cache entries (high TTL) that we would rather have in the cache.

What are the changes in this PR?

Two new configurable parameters:

inter_query_builtin_cache.forced_eviction_threshold_percentage: By default 100, an integer that denotes the percentage of inter_query_builtin_cache.max_size_bytes that cache usage can grow to before cache will start forcefully evicting entries in a FIFO fashion.
inter_query_builtin_cache.stale_entry_eviction_period_seconds: By default 0, an integer that represents the time period in seconds when the stale entry cleanup routine runs.

Notes to assist PR review:

Resolves Improve http.send cache cleaning #5320
Primary changes in cache.go and http.go. Rest are for passing around context and logger objects.

Further comments:

N/A

netlify · 2023-11-07T15:22:28Z

✅ Deploy Preview for openpolicyagent ready!

Name	Link
🔨 Latest commit	`b3586f4`
🔍 Latest deploy log	https://app.netlify.com/sites/openpolicyagent/deploys/65975098f357490008ac0c3a
😎 Deploy Preview	https://deploy-preview-6392--openpolicyagent.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

topdown/cache/cache.go

topdown/http.go

topdown/cache/cache.go

anderseknert · 2023-11-09T13:23:45Z

This is an awesome contribution! Thanks, and keep up the great work! 😃 👏

ashutosh-narkar · 2023-11-13T22:55:06Z

Thanks for the contribution @rudrakhp! I'll review it this week.

ashutosh-narkar

@rudrakhp thanks for the contribution. The main comment I have is we need to make changes that are backwards compatible. Some of the API changes you've proposed are not. We can look into introducing a new type if required for the new functionality and think about deprecating the old one.

topdown/cache/cache.go

johanfylling

LGTM 👍. Thanks for the contribution 😃!

runtime/runtime.go

topdown/cache/cache.go

ashutosh-narkar · 2023-12-21T01:58:55Z

topdown/cache/cache.go

+			defer ticker.Stop()
+			for {
+				select {
+				case <-ticker.C:


What would happen if we still in the middle of a clean up and ticker ticks? We should probably stop the ticker, do the work and recreate it.

@ashutosh-narkar Only one clean up routine runs at a time, since we listen for more ticker events only after the clean up routine returns. But the time period StaleEntryEvictionPeriodSeconds is enforced between start of each routine. We should reset the ticker if we intend to enforce it between end of previous routine and start of the new one.

We should probably stop the ticker, do the work and recreate it.

SGTM, added changes for the same.

ashutosh-narkar

@rudrakhp this is getting close. Few comments inline 👇 Thanks for working on this.

ashutosh-narkar · 2023-12-21T19:58:42Z

topdown/cache/cache.go

+func NewInterQueryCacheWithContext(ctx context.Context, config *Config) InterQueryCache {
+	iqCache := newCache(config)
+	if iqCache.staleEntryEvictionTimePeriodSeconds() > 0 {
+		iqCache.ticker = time.NewTicker(time.Duration(iqCache.staleEntryEvictionTimePeriodSeconds()) * time.Second)


We don't need to set the ticker on the cache struct. It would be cleaner to just define the ticker locally, then stop the ticker in both the select cases and in case <-iqCache.ticker.C you'd just recreate it after the iqCache.cleanStaleValues() call.

ashutosh-narkar · 2023-12-21T20:16:11Z

topdown/http_test.go

@@ -2159,7 +2159,7 @@ func TestNewInterQueryCacheValue(t *testing.T) {
 		Body:       io.NopCloser(bytes.NewBuffer(b)),
 	}

-	result, err := newInterQueryCacheValue(BuiltinContext{}, response, b, &forceCacheParams{})
+	result, _, err := newInterQueryCacheValue(BuiltinContext{}, response, b, &forceCacheParams{})


It would be good if we add some tests which exercise the new optional params.

@ashutosh-narkar Not sure I got this right, if you are talking about the new cache config params, we have unit tests in cache_test.go exercising them already.
http_tests.go wasn't using the new NewInterQueryCacheWithContext and InsertWithExpiry, have updated it now if that's what you meant. Let me know if there is anything more to test in http_test.go.

I mean in the tests in http_test.go we invoke http.send with its params. I was suggesting to add a case where we call http.send with the new params.

ashutosh-narkar · 2023-12-21T20:17:18Z

topdown/cache/cache.go

+				select {
+				case <-iqCache.ticker.C:
+					iqCache.cleanStaleValues()
+				case <-ctx.Done():


A unit test that cancels the context to test this code path would be good.

@ashutosh-narkar Can you check the test TestCancelNewInterQueryCacheWithContext already added? Let me know if you think we can test anything more.

ashutosh-narkar

@rudrakhp thanks for working on this. Just couple of small comments and then we can get this in. It would be great if you could add an explanation about this change in the commit message body.

ashutosh-narkar · 2024-01-05T00:36:20Z

topdown/cache/cache.go

+	if iqCache.staleEntryEvictionTimePeriodSeconds() > 0 {
+		cleanupTicker := time.NewTicker(time.Duration(iqCache.staleEntryEvictionTimePeriodSeconds()) * time.Second)
+		go func() {
+			defer cleanupTicker.Stop()


Nit: We could just move this in the <-ctx.Done() case and avoid the defer statement.

ashutosh-narkar · 2024-01-05T00:53:59Z

topdown/http_test.go

@@ -2159,7 +2159,7 @@ func TestNewInterQueryCacheValue(t *testing.T) {
 		Body:       io.NopCloser(bytes.NewBuffer(b)),
 	}

-	result, err := newInterQueryCacheValue(BuiltinContext{}, response, b, &forceCacheParams{})
+	result, _, err := newInterQueryCacheValue(BuiltinContext{}, response, b, &forceCacheParams{})


I mean in the tests in http_test.go we invoke http.send with its params. I was suggesting to add a case where we call http.send with the new params.

Regularly clean up of cache entries that have expired for a more efficient use of memory. Introduce two new parameters to tune clean up frequency and threshold for forced FIFO eviction. Fixes open-policy-agent#5320 Signed-off-by: Rudrakh Panigrahi <rudrakh97@gmail.com>

ashutosh-narkar

LGTM. Thanks for working on this @rudrakhp!

rudrakhp changed the title ~~Issue-5320 clean expired cache entries periodically~~ [WIP] Issue-5320 clean expired cache entries periodically Nov 7, 2023

rudrakhp force-pushed the clean_expire_cache_entries branch 2 times, most recently from 9689d07 to 3a9499d Compare November 8, 2023 11:11

rudrakhp changed the title ~~[WIP] Issue-5320 clean expired cache entries periodically~~ Issue-5320 clean expired cache entries periodically Nov 8, 2023

rudrakhp marked this pull request as ready for review November 8, 2023 11:23

lukyer reviewed Nov 9, 2023

View reviewed changes

topdown/cache/cache.go Outdated Show resolved Hide resolved

lukyer reviewed Nov 9, 2023

View reviewed changes

topdown/http.go Outdated Show resolved Hide resolved

lukyer reviewed Nov 9, 2023

View reviewed changes

topdown/cache/cache.go Outdated Show resolved Hide resolved

rudrakhp force-pushed the clean_expire_cache_entries branch 2 times, most recently from 500bb33 to cb43b1e Compare November 10, 2023 21:27

rudrakhp requested a review from lukyer November 10, 2023 21:42

rudrakhp force-pushed the clean_expire_cache_entries branch from cb43b1e to ea52d5f Compare November 11, 2023 06:46

ashutosh-narkar force-pushed the clean_expire_cache_entries branch 2 times, most recently from 8af9e58 to 4d2c1fe Compare November 14, 2023 18:41

ashutosh-narkar requested changes Nov 15, 2023

View reviewed changes

topdown/cache/cache.go Outdated Show resolved Hide resolved

topdown/cache/cache.go Outdated Show resolved Hide resolved

rudrakhp force-pushed the clean_expire_cache_entries branch 2 times, most recently from e1de25d to 381b151 Compare November 18, 2023 06:54

rudrakhp requested a review from ashutosh-narkar November 18, 2023 07:44

rudrakhp force-pushed the clean_expire_cache_entries branch 3 times, most recently from 2619e32 to 36b8968 Compare November 21, 2023 18:12

ashutosh-narkar requested review from johanfylling and removed request for lukyer December 1, 2023 03:49

johanfylling previously approved these changes Dec 20, 2023

View reviewed changes

runtime/runtime.go Outdated Show resolved Hide resolved

ashutosh-narkar reviewed Dec 21, 2023

View reviewed changes

rudrakhp dismissed johanfylling’s stale review via 06392ab December 21, 2023 06:22

rudrakhp force-pushed the clean_expire_cache_entries branch from 36b8968 to 06392ab Compare December 21, 2023 06:22

rudrakhp force-pushed the clean_expire_cache_entries branch 6 times, most recently from d5f7c13 to 8117bac Compare December 21, 2023 10:51

ashutosh-narkar reviewed Dec 21, 2023

View reviewed changes

rudrakhp force-pushed the clean_expire_cache_entries branch 2 times, most recently from ac9a974 to 4595908 Compare December 23, 2023 03:51

ashutosh-narkar force-pushed the clean_expire_cache_entries branch from 4595908 to b3586f4 Compare January 5, 2024 00:43

ashutosh-narkar reviewed Jan 5, 2024

View reviewed changes

rudrakhp force-pushed the clean_expire_cache_entries branch from b3586f4 to 1b7b293 Compare January 9, 2024 18:51

rudrakhp force-pushed the clean_expire_cache_entries branch from 1b7b293 to 264ff95 Compare January 9, 2024 19:06

ashutosh-narkar approved these changes Jan 9, 2024

View reviewed changes

ashutosh-narkar merged commit f063c90 into open-policy-agent:main Jan 9, 2024
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue-5320 clean expired cache entries periodically #6392

Issue-5320 clean expired cache entries periodically #6392

rudrakhp commented Nov 7, 2023 •

edited

Loading

netlify bot commented Nov 7, 2023 •

edited

Loading

anderseknert commented Nov 9, 2023

ashutosh-narkar commented Nov 13, 2023

ashutosh-narkar left a comment

johanfylling left a comment

ashutosh-narkar Dec 21, 2023

rudrakhp Dec 21, 2023 •

edited

Loading

ashutosh-narkar left a comment

ashutosh-narkar Dec 21, 2023

ashutosh-narkar Dec 21, 2023

rudrakhp Dec 22, 2023 •

edited

Loading

ashutosh-narkar Jan 5, 2024

ashutosh-narkar Dec 21, 2023

rudrakhp Dec 22, 2023

ashutosh-narkar left a comment

ashutosh-narkar Jan 5, 2024

ashutosh-narkar Jan 5, 2024

ashutosh-narkar left a comment

Issue-5320 clean expired cache entries periodically #6392

Issue-5320 clean expired cache entries periodically #6392

Conversation

rudrakhp commented Nov 7, 2023 • edited Loading

Why the changes in this PR are needed?

What are the changes in this PR?

Notes to assist PR review:

Further comments:

netlify bot commented Nov 7, 2023 • edited Loading

✅ Deploy Preview for openpolicyagent ready!

anderseknert commented Nov 9, 2023

ashutosh-narkar commented Nov 13, 2023

ashutosh-narkar left a comment

Choose a reason for hiding this comment

johanfylling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudrakhp Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

ashutosh-narkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudrakhp Dec 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutosh-narkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutosh-narkar left a comment

Choose a reason for hiding this comment

rudrakhp commented Nov 7, 2023 •

edited

Loading

netlify bot commented Nov 7, 2023 •

edited

Loading

rudrakhp Dec 21, 2023 •

edited

Loading

rudrakhp Dec 22, 2023 •

edited

Loading