Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15306 )
Change subject: IMPALA-8690: Add LIRS cache eviction algorithm ...................................................................... Patch Set 21: (5 comments) Working on a new upload (which will add comments). Going ahead and posting replies. http://gerrit.cloudera.org:8080/#/c/15306/21/be/src/util/cache/lirs-cache.cc File be/src/util/cache/lirs-cache.cc: http://gerrit.cloudera.org:8080/#/c/15306/21/be/src/util/cache/lirs-cache.cc@131 PS21, Line 131: entires > nit: typo Will fix http://gerrit.cloudera.org:8080/#/c/15306/21/be/src/util/cache/lirs-cache.cc@456 PS21, Line 456: mem_tracker_->Consume(deferred_consumption_); > shouldn't all the memory be release when the shard is destroyed? I will add a comment here. The eviction/free code in CleanupThreadState() is calling UpdateMemTracker() as it goes, which decrements the underlying memtracker each time it exceeds max_deferred_consumption. The most that can be left over at this point is the deferred_consumption. http://gerrit.cloudera.org:8080/#/c/15306/21/be/src/util/cache/lirs-cache.cc@861 PS21, Line 861: e->set_resident(); > are we suppose to set that data as resident even if 'success' is false? The failure condition is like being successful and then evicting it immediately. So, we do all the same preparation up front. The contract for a caller is that the data needs to resident before we call this. The caller ran Allocate() and got an UNINITIALIZED handle with the key filled in. Caller put the data somewhere (like the data cache file) and then filled in the value on the handle. Now it is calling Insert(), and the data is already resident. If the Insert fails, it calls the eviction callback, which will make this not resident (and also decrement the memtracker below). I will add a comment here to clarify the success/failure case and how it works. http://gerrit.cloudera.org:8080/#/c/15306/21/be/src/util/cache/lirs-cache.cc@871 PS21, Line 871: // 3. There is a non-tombstone entry (rare) > is this possible? i guess this could happen if two Inserts of the same data Yes, this is possible in a race condition on insert. It is also possible for the data cache when we are inserting a larger version of an existing entry (i.e. file=X offset=0 length=5 is getting replaced by file=X offset=0 length=10). http://gerrit.cloudera.org:8080/#/c/15306/21/be/src/util/cache/lirs-cache.cc@876 PS21, Line 876: UpdateMemTracker(e->charge()); > not sure I actually understand what this is doing, so perhaps a misplaced c See other comment on residency. This is the same between success/failure, because eviction will decrement this. -- To view, visit http://gerrit.cloudera.org:8080/15306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I670fa4b2b7c93998130dc4e8b2546bb93e9a84f8 Gerrit-Change-Number: 15306 Gerrit-PatchSet: 21 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: Fri, 27 Mar 2020 19:39:29 +0000 Gerrit-HasComments: Yes
