Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22181 )
Change subject: KUDU-613: Remove use of vectors under lock ...................................................................... Patch Set 3: (5 comments) Almost there. Just a few nits to address. http://gerrit.cloudera.org:8080/#/c/22181/4/src/kudu/util/slru_cache.h File src/kudu/util/slru_cache.h: http://gerrit.cloudera.org:8080/#/c/22181/4/src/kudu/util/slru_cache.h@160 PS4, Line 160: UHandle* SoftRemoveEntry(); How to enforce that? Does it make sense to add corresponding DCHECK()? 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 if past capacity. > I'll update the description to clarify that this method should only be call If it were the same level call site, with in-lining SoftRemoveEntry() into the code the logic wouldn't become any harder to digest, IMO. Especially if adding comments for the code. But the crucial point here is the fact that this logic is a part of a shard, but it's called at the upper level :) So there isn't a cleaner way to deal with it then, and I'm totally fine with it as-is. Sorry for the noise. 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: > I don't think it can be the same name, but given this is the update case, I I'd think it should be possible to use the same Insert() name. That would be just a specialization for the method for Segment::kProtected parameter type. Did you try and it failed compiling or so? http://gerrit.cloudera.org:8080/#/c/22181/4/src/kudu/util/slru_cache.cc File src/kudu/util/slru_cache.cc: http://gerrit.cloudera.org:8080/#/c/22181/4/src/kudu/util/slru_cache.cc@309 PS4, Line 309: ProtectedIns If renaming this into UpgradeInsert() why not to do so for SLRUCacheShard<Segment::kProbationary>::Insert() as well? IIUC, the semantics of the methods are the same, and you can use the template specialization to have different implementations of a method with the same signature for different template parameters. Otherwise, maybe get back to SLRUCacheShard<Segment::kProtected>::Insert() naming as well for the protected shards? http://gerrit.cloudera.org:8080/#/c/22181/4/src/kudu/util/slru_cache.cc@493 PS4, Line 493: probationary_shard_.ReInsert(down DCHECK_NOTNULL is useful when using the result, if you aren't using it in the code, there is a simpler DCHECK(...) or DCHECK(... != nullptr), where the former is preferred. -- 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: 3 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 23:23:37 +0000 Gerrit-HasComments: Yes
