Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20607 )
Change subject: WIP KUDU-613: Scan Resistant Caching ...................................................................... Patch Set 1: (7 comments) Overall this approach makes sense to me. At this point, it seems there is at least an issue with eviction callbacks, if I'm not mistaken. It seems more attention needs to be paid to the overall flow of those when an entry is evicted from a region/sub-cache of the SLRU cache. http://gerrit.cloudera.org:8080/#/c/20607/1/src/kudu/util/cache.cc File src/kudu/util/cache.cc: http://gerrit.cloudera.org:8080/#/c/20607/1/src/kudu/util/cache.cc@546 PS1, Line 546: FreeEntry(to_remove_head); In this context, I'd think of having more control over the eviction callback that's a part of the FreeEntry() call. IIUC, the actual eviction could happen later on when the entry is definitely pushed out of the SLRU cache, but at this time it could not be possible to tell whether . What do you think? http://gerrit.cloudera.org:8080/#/c/20607/1/src/kudu/util/cache.cc@780 PS1, Line 780: explicit Do we really need explicit here? http://gerrit.cloudera.org:8080/#/c/20607/1/src/kudu/util/cache.cc@780 PS1, Line 780: ShardedSLRUCache Would be nice to document the parameters of the constructor (e.g., what capacity units are used). BTW, do we really need to expose two separate ID strings for the part of the cache? Would it be enough to have just a single ID string, and the ID strings for the probationary and the protected parts might be auto-generated based on that single ID string? http://gerrit.cloudera.org:8080/#/c/20607/1/src/kudu/util/cache.cc@797 PS1, Line 797: insert them into probationary cache Would be great to clarify whether that insert is constrained by the capacity of the probationary part of the cache. http://gerrit.cloudera.org:8080/#/c/20607/1/src/kudu/util/cache.cc@797 PS1, Line 797: allocate Do we really need to allocate anything at that point? http://gerrit.cloudera.org:8080/#/c/20607/1/src/kudu/util/cache.cc@810 PS1, Line 810: Erase It seems Erase() might trigger calling the eviction callback, and that's not the expected behavior, I guess. http://gerrit.cloudera.org:8080/#/c/20607/1/src/kudu/util/nvm_cache.cc File src/kudu/util/nvm_cache.cc: http://gerrit.cloudera.org:8080/#/c/20607/1/src/kudu/util/nvm_cache.cc@339 PS1, Line 339: vector<Cache::Handle*> It would be great to clarify on the ownership of the returned items in the inline doc comment. -- To view, visit http://gerrit.cloudera.org:8080/20607 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I45531534a2049dd38c002f4dc7982df9fd46e0bb Gerrit-Change-Number: 20607 Gerrit-PatchSet: 1 Gerrit-Owner: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Mon, 23 Oct 2023 18:31:46 +0000 Gerrit-HasComments: Yes
