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

Added a function to evict all elements older than the cache TTL #5464

Merged
merged 15 commits into from
Dec 5, 2023

Conversation

jakobht
Copy link
Member

@jakobht jakobht commented Nov 28, 2023

What changed?
Added a function to evict all elements in the LRU cache older than the TTL

Why?
This will enable us to clean out the cache periodically. This is useful for the workflow specific rate limits, as we will only need to keep items which are accessed very frequently.

We will set the TTL to a low value and periodically clear out the elements that are above this limit to save memory.

This way there is no need to set and manage a specific cache size

How did you test it?
Unit tests

Potential risks
No risk, just adding a function

Release notes

Documentation Changes

common/cache/lru.go Outdated Show resolved Hide resolved
@@ -384,3 +384,74 @@ func TestPanicOptionsIsNil(t *testing.T) {

New(nil)
}

func TestEvictItemsPastTimeToLive_AllExpired(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

we are going to slowly switch to table driven tests. please rewrite these new tests using that pattern https://golang.cafe/blog/golang-table-test-example

Copy link
Member

Choose a reason for hiding this comment

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

Agree on this case. Though, can add a rant:
There is a small article on test naming from one of already ex Uber engineers: https://github.com/recht/rants/blob/master/testing.md

and style guide https://github.com/uber-go/guide/blob/master/style.md#avoid-unnecessary-complexity-in-table-tests
I would say that table tests can work, it is not a silver bullet.

Copy link
Member

Choose a reason for hiding this comment

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

I would say table tests work very well compared to the other approaches. Some benefits are

  • Maintainability: For the same unit of work you write the body once and you can easily add more cases without making redundant copy of the validation logic. If you need to change something in your test body you have to change it in N different places without table driven tests.
  • Readability: Creating structs for test cases makes it very clear to understand what are the inputs and expected outputs. Much better readability.

Scope of your test function and what you put in the test body can of course increase complexity but that can and does happen in other approaches. Style guide has a good point about not combining irrelevant cases together.

Copy link
Member

@3vilhamster 3vilhamster Nov 30, 2023

Choose a reason for hiding this comment

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

From my experience:
Table test works great for a set of similar cases that can be easily defined.
But every complication requires modification of initial struct and very fast it becomes hard to maintain/understand. I guess I've see a lot of bad cases from the combining irrelevant cases, or not keeping a single responsibility principle.

Lru is pretty complicated and codifying steps will be harder than just expressing what you are trying to test. At least the way I see how it could be codified.

Copy link
Member

Choose a reason for hiding this comment

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

To be clear there's no suggestion to fit all of the lru_test.go into single tabular function to simulate all the scenarios.
Different scenarios deserve their own functions which simulate something we are interested to validate.

Regarding the new functions in this PR, all lines are duplicate except ~1-2 line difference (either an extra sleep or calling EvictItemsPastTimeToLive vs Size). This can be turned into table format easily.

common/cache/cache.go Outdated Show resolved Hide resolved
common/cache/lru.go Outdated Show resolved Hide resolved
common/cache/lru_test.go Outdated Show resolved Hide resolved
common/cache/lru.go Outdated Show resolved Hide resolved
common/cache/lru.go Outdated Show resolved Hide resolved
common/cache/lru.go Outdated Show resolved Hide resolved
common/cache/lru.go Outdated Show resolved Hide resolved
@jakobht jakobht enabled auto-merge (squash) December 5, 2023 11:23
@jakobht jakobht merged commit 42bdd9e into cadence-workflow:master Dec 5, 2023
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants