Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/22181 )
Change subject: KUDU-613: Remove use of vectors under lock ...................................................................... Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/22181/3/src/kudu/util/slru_cache.h File src/kudu/util/slru_cache.h: http://gerrit.cloudera.org:8080/#/c/22181/3/src/kudu/util/slru_cache.h@159 PS3, Line 159: // Removes LRU entry of the protected segment and returns handle. > Please clarify what happens if there aren't any entries past capacity -- wh I'll update the description to clarify that this method should only be called when the shard's usage exceeds the shard's capacity. After the latest refactoring, this function is only called once, that is correct. That being said, I prefer to separate out the logic like this. The reason being is the logic within DowngradeEntries() is a lot simpler to digest now, with one line for the entry being removed, and another being reinserted into the probationary shard. Also, the logic within this function is on the shard level, while its only callsite is on the shardPair level. I prefer to separate out the logic if possible. Let me know what you think, thanks. http://gerrit.cloudera.org:8080/#/c/22181/3/src/kudu/util/slru_cache.h@225 PS3, Line 225: Remove any entries past capacity in the > nit: does it make sense to to clarify that entries past the probationary sh Done http://gerrit.cloudera.org:8080/#/c/22181/3/src/kudu/util/slru_cache.cc File src/kudu/util/slru_cache.cc: http://gerrit.cloudera.org:8080/#/c/22181/3/src/kudu/util/slru_cache.cc@a317 PS3, Line 317: > Since you changing the names of the methods (like UpgradeInsert() above), m I don't think it can be the same name, but given this is the update case, I think a more appropriate name is UpdateInsert(). I'll omit the Protected part since this is a template specialization for Segment::kProtected. http://gerrit.cloudera.org:8080/#/c/22181/3/src/kudu/util/slru_cache.cc@492 PS3, Line 492: auto* downgraded_entry = protected_shard_.SoftRemoveEntry(); > nit: does it make sense to add a DCHECK() on non-null of downgraded_entry h Done -- To view, visit http://gerrit.cloudera.org:8080/22181 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8f6a019c7c033c6c3dfef59b0f037e78f8264e68 Gerrit-Change-Number: 22181 Gerrit-PatchSet: 4 Gerrit-Owner: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Comment-Date: Mon, 09 Dec 2024 21:26:03 +0000 Gerrit-HasComments: Yes
