-
Notifications
You must be signed in to change notification settings - Fork 138
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
CBG-4323: fix for per shard memory based eviction #7174
Conversation
Redocly previews |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good with some naming/comment suggestions for clarity and a suggestion for a future enhancement ticket.
db/revision_cache_lru.go
Outdated
cacheNumItems *base.SgwIntStat | ||
lock sync.Mutex | ||
capacity uint32 // Max number of items capacity of LRURevisionCache | ||
MaxMemoryCapacity int64 // Max memory capacity of LRURevisionCache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't look like this needs to be a public property (can be maxMemoryCapacity
). Actually - I'm not clear why this needed to change from memoryCapacity
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making public was mistake, the renaming was just to make it more clear in terms of distinction from currMemoryCapacity
but after the rename of that to currMemoryUsage
I can revert it.
// LRURevisionCache object for sharding the rev cache. This way we can perform eviction on per shard basis much like | ||
// we do with the number of items capacity eviction | ||
rc.currMemoryCapacity.Add(bytesCount) | ||
rc.cacheMemoryBytesStat.Add(bytesCount) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other scenarios like this (for the channel cache in particular), we compute aggregate stats at stat collection time (see UpdateCalculatedStats). The above approach may be fine for 3.2.1 (based on performance results), but filing a tracking ticket to use this approach going forward would be a good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the approach I wanted to take, but it came with its complexities with fetching the current memory usage. Would require a new interface method on the rev cache which I thought at this time may not be wanted (given the time constraints). I can file a ticket to explore this in a future release.
CBG-4323
Pre-review checklist
fmt.Print
,log.Print
, ...)base.UD(docID)
,base.MD(dbName)
)docs/api
Integration Tests
GSI=true,xattrs=true
https://jenkins.sgwdev.com/job/SyncGateway-Integration/2764/