Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20607 )
Change subject: KUDU-613: Introduce SLRU cache ...................................................................... Patch Set 12: (11 comments) http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache-test.cc File src/kudu/util/slru_cache-test.cc: http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache-test.cc@446 PS12, Line 446: Consider adding the following test scenarios as well: * trying to insert an entry with the same key into SLRU cache: (sub-case A) the entry is in probation segment (sub-case B) the entry is in protected segment; in both sub-cases add a sub-sub-case to work with relatively tiny entries (1/10 of the segment capacity) and large entries (1/2, 1/3, and 1/1 of the segment size) * insert-and-then-lookup for the same key: do it multiple times, up to and over the lookup threshold http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.h File src/kudu/util/slru_cache.h: http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.h@40 PS12, Line 40: SLRUCache It seems there are many common methods between the Cache's and SLRUCache's interfaces. Does it make sense to have SLRUCache inheriting from Cache? Is it really necessary to put SLRUCache in a separate inheritance branch? http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.h@121 PS12, Line 121: // Returns true if the probationary segment contains key, false if not. : virtual bool ProbationaryContains(const Slice& key) = 0; : : // Returns true if the protected segment contains key, false if not. : virtual bool ProtectedContains(const Slice& key) = 0; Why to have these two methods in the public interface? If only the implementation and tests use these, maybe make them private and add the corresponding tests into the list of friends. Also, why not to have those just at the level of SLRUShardedCache? http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.h@134 PS12, Line 134: : private: : DISALLOW_COPY_AND_ASSIGN(SLRUCache); Consider moving this into the concrete class implementation: SRLUCache is just an interface, right? http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc File src/kudu/util/slru_cache.cc: http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@507 PS12, Line 507: void SetProbationaryCapacity(size_t probationary_capacity) { : probationary_shard_->SetCapacity(probationary_capacity); : } : : void SetProtectedCapacity(size_t protected_capacity) { : protected_shard_->SetCapacity(protected_capacity); : } Why to have these as methods instead of supplying the capacity for the probationary and the protected segments as parameters for the SLRUCacheShardPair's constructor? So far it seems these methods are used only right after an instance of SLRUCacheShardPair is created, before adding it into the 'shards_' container of the ShardedSLRUCache class. http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@526 PS12, Line 526: SLRUCacheShard* probationary_shard_; : SLRUCacheShard* protected_shard_; Why to have these created on the heap, not on the stack? Please add a comment with the reasoning. If having these on the heap, why to keep the raw pointers instead wrapping them into std::unique_ptr? http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@529 PS12, Line 529: the following state What state? http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@556 PS12, Line 556: size_t i = evictions.size(); i > 0; --i nit: use reverse iterator instead? http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@558 PS12, Line 558: evicted_entry->lookups = 0; : evicted_entry->prev = nullptr; : evicted_entry->next = nullptr; : evicted_entry->next_hash = nullptr; Does it make sense to move this sanitization into the ReInsert() method? http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@627 PS12, Line 627: if (ProbationaryContains(e->key(), e->hash)) { : probationary_shard_->Release(handle); : } else { : protected_shard_->Release(handle); : } Is it possible to have any race between the ProbationaryContains() check and some other thread that might move the entry between the segments? If not, please add a comment explaining why there cannot be any http://gerrit.cloudera.org:8080/#/c/20607/12/src/kudu/util/slru_cache.cc@800 PS12, Line 800: SLRUCacheShardPair* Is it possible to store unique_ptr instead of raw pointers in the shards_ container? -- 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: 12 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: Wed, 14 Feb 2024 22:28:52 +0000 Gerrit-HasComments: Yes
