bugfix: cache get function, when in pin mode, should not increase the… #665
Conversation
… counter before return
| if c.pin { | ||
| entry.refCount++ | ||
| } | ||
|
|
||
| if c.isEntryExpired(entry, time.Now()) { | ||
| // Entry has expired |
There was a problem hiding this comment.
I'm confused. c.deleteInternal(element) apparently deletes the entry with associated refCount, so it doesn't matter if it is incremented or not.
There was a problem hiding this comment.
isEntryExpired actually check whether the ref count is 0 or not:
https://github.com/uber/cadence/pull/665/files/5f8ad70e36c2d500163634b7c3c6199638c38877#diff-15d63b195284f9a26bdbeb5e40398165R303
There was a problem hiding this comment.
I see. Then the question is why it wasn't found before? It looks like we don't have an expiration test on staging.
There was a problem hiding this comment.
https://github.com/uber/cadence/pull/665/files#diff-15d63b195284f9a26bdbeb5e40398165R279
on the put side, the logic is sound.
hence, when the cache is full, the least used entry is deleted.
There was a problem hiding this comment.
moreover, there was test conducted in staging, testing the history service memory usage, since the put side logic is fine, this issue is not detected.
… counter before return