Sahil Takiar 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) 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 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? 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? 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 happen concurrently? 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 comment, but what happens if 'success' is false? -- 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 17:31:36 +0000 Gerrit-HasComments: Yes
