Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20607 )
Change subject: KUDU-613: Introduce SLRU cache ...................................................................... Patch Set 19: (2 comments) http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.cc File src/kudu/util/slru_cache.cc: http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.cc@66 PS19, Line 66: vector<Cache::RLHandle*> InsertAndReturnEvicted(Cache::RLHandle* handle); For this and other methods that are specific to protected or probational segments: it would be great if those were present only in classes for corresponding segments, otherwise the code is bit brittle since corresponding invariants aren't checked. I'm not sure whether you have capacity to take care of this in this patch, but for the future consider either using one extra level of inheritance (ProbationalShardSegment --> SLRUCacheShard, ProtectedShardSegment --> SLRUCacheShard) to have those specific methods only in particular shards, or maybe using templatized approach and std::enable_if template meta-programming construct. I'm OK with keeping it as-is in this patch, maybe just add TODO to make the code more robust. http://gerrit.cloudera.org:8080/#/c/20607/19/src/kudu/util/slru_cache.cc@468 PS19, Line 468: // Go through all evicted entries from the protected segment and insert them into : // the probationary segment. Reverse iterate through evictions to insert MRU first. : for (auto it = evictions.rbegin(); it != evictions.rend(); ++it) { : Cache::RLHandle* evicted_entry = *it; : probationary_shard_.ReInsert(evicted_entry); : } After consideration, inserting MRU first seems counter-productive. For simplicity, let's assume the target segment is empty before all this elements start being inserted. If MRU are inserted first, and LRU inserted last, then: * among all the inserted, MRU becomes LRU after insertion is complete * if there isn't enough space in the segment, then LRU elements that are inserted last would push out elements inserted earlier (i.e. those that used to be MRU elements), which means MRU elements be evicted, but LRU elements stay I might be missing something, please correct me if I'm wrong. -- 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: 19 Gerrit-Owner: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <mre...@cloudera.com> Gerrit-Reviewer: Marton Greber <greber...@gmail.com> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Sat, 01 Jun 2024 06:24:43 +0000 Gerrit-HasComments: Yes
