Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/20607 )
Change subject: WIP KUDU-613: Scan Resistant Caching ...................................................................... Patch Set 2: (7 comments) Thanks for your feedback! I left comments addressing your concerns about the eviction callbacks. For the block cache, there's no callback interface used anyways as a null pointer is passed in for the eviction_callback object when inserting into a 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: to_remove_head = next; > In this context, I'd think of having more control over the eviction callbac The actual handle for this entry isn't being returned to insert into the probationary cache. Only the necessary information from the evicted entries are being returned as part of EvictedHandle, so it's fine if the eviction callback is called now since it would just delete the entry from the protected cache, then we construct a new entry for the probationary cache. http://gerrit.cloudera.org:8080/#/c/20607/1/src/kudu/util/cache.cc@780 PS1, Line 780: / 'prote > Do we really need explicit here? Done http://gerrit.cloudera.org:8080/#/c/20607/1/src/kudu/util/cache.cc@780 PS1, Line 780: ted-capacity'. T > Would be nice to document the parameters of the constructor (e.g., what cap Good point, changed it to one id and auto generated ids for sub caches http://gerrit.cloudera.org:8080/#/c/20607/1/src/kudu/util/cache.cc@797 PS1, Line 797: s are evicted from protected cache, > Would be great to clarify whether that insert is constrained by the capacit Yep the insert is constrained by the capacity, but the cache will evict as many entries as necessary to be able to insert the new entry while being within the capacity of the cache. That's all done as part of the Insert() method. http://gerrit.cloudera.org:8080/#/c/20607/1/src/kudu/util/cache.cc@797 PS1, Line 797: If any e > Do we really need to allocate anything at that point? When entries are evicted from the protected cache, the logic I wrote right now only returns the necessary information to build a new entry by using the Allocate/Insert methods. So the actual entry gets erased from the protected cache, but we retain the necessary information to build a new handle for the same entry. The alternative to this would be to retain the actual handle that points to the entry, but when it's being evicted FreeEntry() is called which deletes the data. I could add a reference to this handle which would prevent the data from being deleted, some other information (the hash) would have to be changed to reflect the entry being in a new cache (probationary). Instead of doing this, I thought it would be more straightforward/less error prone to retain the necessary information to build a new entry and go through the insertion path again by calling Allocate() and Insert(). http://gerrit.cloudera.org:8080/#/c/20607/1/src/kudu/util/cache.cc@810 PS1, Line 810: obati > It seems Erase() might trigger calling the eviction callback, and that's no If the eviction callback was defined, it would just evict it from the probationary cache, not the entire SLRU cache. So that behavior is consistent with what we expect here. I actually have to move this logic after the allocation and insertion of this entry into the protected cache, since the allocation/insertion uses the handle returned from a Get call() which returns a pointer to the entry. If we erase this entry here, then IIUC the value would be erased. 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: // Useful for SLRU cac > It would be great to clarify on the ownership of the returned items in the Done -- 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: 2 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 25 Oct 2023 20:22:49 +0000 Gerrit-HasComments: Yes
