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 6: (5 comments) 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: Update the memtracker's consumption by the given amount. > How to enforce that? Does it make sense to add corresponding DCHECK()? Removed the function, no longer relevant. 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: > If it were the same level call site, with in-lining SoftRemoveEntry() into Given the while loop condition already deals with shard-level logic, I decided to remove this function and just insert the logic directly into DowngradeEntries(). This also lets us avoid adding a redundant DCHECK in SoftRemoveEntry() given the while loop already checks that the usage > capacity. 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'd think it should be possible to use the same Insert() name. That would Ah I misunderstood your original message. Yes that's possible, I can also do the same for the ReInsert and UpgradeInsert methods. Given it's outside the scope of this patch, I'll address it in a follow-up patch. 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: > If renaming this into UpgradeInsert() why not to do so for SLRUCacheShard<S Yeh I'll address this in a follow-up patch, along with ReInsert and UprgadeInsert to use template specialization for both functions. http://gerrit.cloudera.org:8080/#/c/22181/4/src/kudu/util/slru_cache.cc@493 PS4, Line 493: > DCHECK_NOTNULL is useful when using the result, if you aren't using it in t 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: 6 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: Tue, 10 Dec 2024 20:06:30 +0000 Gerrit-HasComments: Yes
