Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15179 )
Change subject: IMPALA-8690 (prep 3): Factor out common code for cache implementations ...................................................................... Patch Set 7: (12 comments) http://gerrit.cloudera.org:8080/#/c/15179/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15179/7//COMMIT_MSG@23 PS7, Line 23: need > nit: "needs" Done http://gerrit.cloudera.org:8080/#/c/15179/7//COMMIT_MSG@24 PS7, Line 24: a > nit: delete Done http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h File be/src/util/cache/cache-internal.h: http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h@30 PS7, Line 30: point to > nit: point to what exactly? Reworked this a bit to describe how the contiguous layout would work. http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h@34 PS7, Line 34: HandleBase(uint8_t* kv_ptr, const Slice& key, int32_t hash, int val_len, int charge) { > Might be useful to have a comment, eg. "kv_ptr points to memory allocated t Added a comment to try to explain what is expected for kv_ptr. http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h@46 PS7, Line 46: > formatting Done http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h@68 PS7, Line 68: friend class HandleTable; > nit: formatting Done http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache-internal.h@215 PS7, Line 215: mem_tracker_ = kudu::MemTracker::FindOrCreateGlobalTracker( > Do you know how, if at all, we integration this with our own mem tracking, Right now, I checked and it doesn't integrate. This is actually a good thing, because we are accounting the whole cache usage which is larger than memory. My plan for this is that the mem tracker will change to an Impala version, and it will track only the metadata's memory usage (not on disk cache usage). http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc File be/src/util/cache/cache.cc: http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc@a79 PS7, Line 79: > I think this comment is still true and useful, wrt HandleBase::kv_data_ptr_ Brought this back for kv_data_ptr_. http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc@a554 PS7, Line 554: > This appears to have been dropped. Is it no longer the case? I think the -1 comes from the one byte that is already part of sizeof(RLHandle): uint8_t kv_data[1]; Since we no longer have that entry, I removed the -1. http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc@a560 PS7, Line 560: > I think this comment may also still be useful? Added this back to RLCacheShard::Allocate(). http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc@83 PS7, Line 83: A single shard of a recency list eviction policy > nit: revise to "A single shared of a cache that uses a recency list based e Done http://gerrit.cloudera.org:8080/#/c/15179/7/be/src/util/cache/cache.cc@84 PS7, Line 84: template<Cache::EvictionPolicy policy> : class RLCacheShard : public CacheShard > perhaps this can be addressed later, but it looks like this implementation Yes, I think this cache implementation will only ever do FIFO/LRU. Other algorithms will implement the CacheShard interface, but they are unlikely to share code with this. I think this will not compile with something other than FIFO/LRU due to RL_UpdateAfterLookup only have implementations for FIFO/LRU, but I also added a static_assert that RLCacheShard is only used for FIFO/LRU. The next patch adds LIRS, so it will give a chance to fix anything that ends up looking wrong. -- To view, visit http://gerrit.cloudera.org:8080/15179 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I67294244a3e8a2812f1482fe786bf7f8e6ce054e Gerrit-Change-Number: 15179 Gerrit-PatchSet: 7 Gerrit-Owner: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Comment-Date: Mon, 24 Feb 2020 23:44:12 +0000 Gerrit-HasComments: Yes
