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

Increase poller history LRU cache size #4033

Merged

Conversation

mantas-sidlauskas
Copy link
Contributor

What changed?
LRU poller cache size is increased from 1000 to 5000

Why?
Task list poller count metric is representing entries in this cache. When there are more than 1000 workers, this metric becomes misleading.

How did you test it?
Unit tests

Potential risks

@meiliang86
Copy link
Contributor

Unrelated to this diff, why do we pass in historyUpdatedFunc? IMO we should just pass in metrics scope and emit it inside the cache.
Also, why not calling the cache.Size() method directly to get the size? Calling iterator to just get the size does not sound efficient.

@meiliang86 meiliang86 requested a review from a team March 3, 2021 18:28
@coveralls
Copy link

coveralls commented Mar 3, 2021

Coverage Status

Coverage decreased (-0.08%) to 66.634% when pulling b8968f6 on mantas-sidlauskas:history_size into f2f10d4 on uber:master.

@@ -30,7 +30,7 @@ import (

const (
pollerHistoryInitSize = 0
pollerHistoryInitMaxSize = 1000
pollerHistoryInitMaxSize = 5000
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is per tasklist right? 5000 should be a sufficient large value to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is per tasklist. 5000 should be enough for most cases, if we will see 5000 pollers per tasklist, it might be a good indicator to turn on tasklist sharding

@mantas-sidlauskas
Copy link
Contributor Author

Unrelated to this diff, why do we pass in historyUpdatedFunc? IMO we should just pass in metrics scope and emit it inside the cache.
Also, why not calling the cache.Size() method directly to get the size? Calling iterator to just get the size does not sound efficient.

Yes, this sounds reasonable. Unfortunately current LRU cache implementation evicts outdated entries only when iterating over items (i.e. get list of pollers)

This can be addressed in a separate PR

@mantas-sidlauskas mantas-sidlauskas merged commit 150911c into cadence-workflow:master Mar 5, 2021
@mantas-sidlauskas mantas-sidlauskas deleted the history_size branch March 5, 2021 17:37
yux0 pushed a commit to yux0/cadence that referenced this pull request May 4, 2021
Co-authored-by: Liang Mei <meiliang86@gmail.com>
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.

5 participants