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.

Sign up for GitHub

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

Jump to bottom

LFU Cache Fixes #7479

Merged
merged 9 commits into from Feb 12, 2021
Merged

LFU Cache Fixes #7479

merged 9 commits into from Feb 12, 2021

Conversation

vmg
Copy link
Collaborator

@vmg vmg commented Feb 10, 2021

Description

Some bug fixes, refactorings and more benchmarks for the LFU implementation. Getting it in better shape before we switch to it by default.

Related Issue(s)

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build/CI
  • VTAdmin

@vmg
bloom: fix constructor
a2e6f52 
The existing API for the Bloom Filter constructor was, huh, not ideal.
There is no need to use variable args and check the values of the args
for their meaning.

Signed-off-by: Vicent Marti <vmg@strn.cat>
@vmg
query_engine: Implement contention benchmark
8c3580a 
Signed-off-by: Vicent Marti <vmg@strn.cat>
@vmg
query_engine: add a throughput benchmark
a21d244 
Signed-off-by: Vicent Marti <vmg@strn.cat>
@vmg
cache: enable LFU by default
e04aa3f 
Signed-off-by: Vicent Marti <vmg@strn.cat>
@vmg
dev: ensure dev.env is posix compatible
cf525df 
the '==' operator is not posix; you're supposed to use a single '='

Signed-off-by: Vicent Marti <vmg@strn.cat>
@deepthi
Copy link
Member

deepthi commented Feb 10, 2021

One thing I missed while reviewing the earlier PR was the names of the flags:

  -gate_query_cache_lfu
  -gate_query_cache_memory int
  -gate_query_cache_size int

There is no need to prefix the flag names with gate. Do you mind renaming them?

@vmg
cache: make endtoend tests more resilient
2cd3611 
Signed-off-by: Vicent Marti <vmg@strn.cat>
@vmg
Copy link
Collaborator Author

vmg commented Feb 11, 2021

@deepthi: -gate_query_cache_size int is not a new flag, it exists in the current vtgate, which is the reason why the two new flags I introduced are also prefixed with gate_. Changing gate_query_cache_size would be a breaking change. Should I change it anyway? Or change only the two new flags?

@vmg
endtoend: delay execution
a5b9f1b 
Signed-off-by: Vicent Marti <vmg@strn.cat>
@vmg
query_engine: typo
b197d61 
Signed-off-by: Vicent Marti <vmg@strn.cat>
@vmg
Copy link
Collaborator Author

vmg commented Feb 11, 2021

OK, fixed and fixed all the flappy tests. I think this PR, which enables the LFU cache by default, is ready to merge. ✨

@deepthi
Copy link
Member

deepthi commented Feb 11, 2021 •
edited

@deepthi: -gate_query_cache_size int is not a new flag, it exists in the current vtgate, which is the reason why the two new flags I introduced are also prefixed with gate_. Changing gate_query_cache_size would be a breaking change. Should I change it anyway? Or change only the two new flags?

Oops, I did not realize that one of the flags was already there. I would not hold up this PR. We can rename the flags in a backwards-compatible way in a separate PR (if we decide it is worth doing).

systay
systay reviewed Feb 12, 2021
View reviewed changes
go/cache/cache.go
@@ -76,5 +76,5 @@ type Config struct {
var DefaultConfig = &Config{
MaxEntries: 5000,
MaxMemoryUsage: 32 * 1024 * 1024,
LFU: false,
LFU: true,
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

systay
systay reviewed Feb 12, 2021
View reviewed changes
go/cache/ristretto/bloom/bbloom.go
return uint64(size), uint64(locs)
// NewBloomFilterWithErrorRate returns a new bloomfilter with optimal size for the given
// error rate
func NewBloomFilterWithErrorRate(numEntries uint64, wrongs float64) *Bloom {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@vmg
endtoend: uncomment test
e1d8a81 
Signed-off-by: Vicent Marti <vmg@strn.cat>
@systay systay merged commit a577cc4 into vitessio:master Feb 12, 2021
@askdba askdba added this to the v10.0 milestone Feb 15, 2021
@deepthi deepthi mentioned this pull request Mar 12, 2021
Open
@ajm188 ajm188 mentioned this pull request Jul 14, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@systay systay systay approved these changes

@harshit-gangal harshit-gangal Awaiting requested review from harshit-gangal

@shlomi-noach shlomi-noach Awaiting requested review from shlomi-noach

@sougou sougou Awaiting requested review from sougou

Assignees
No one assigned
Labels
Component: Query Serving
Projects
None yet
Milestone
v10.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@vmg @deepthi @systay @askdba