Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15971 )
Change subject: [master] cache for table locations ...................................................................... Patch Set 10: (6 comments) http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/integration-tests/table_locations-itest.cc File src/kudu/integration-tests/table_locations-itest.cc: http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/integration-tests/table_locations-itest.cc@1081 PS10, Line 1081: void TearDown() override { : if (cluster_) { : cluster_->Shutdown(); : cluster_.reset(); : } : KuduTest::TearDown(); : } nit: doesn't this happen anyway via cluster_'s destructor? http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/integration-tests/table_locations-itest.cc@1149 PS10, Line 1149: NO_FATALS(CheckCacheMetricsResetAllMasters()); nit: Since this is only used once, perhaps inline it? http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h File src/kudu/master/table_locations_cache.h: http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h@54 PS10, Line 54: // Copying of entry handles is explicitly prohibited. : EntryHandle(const EntryHandle&) = delete; : EntryHandle& operator=(const EntryHandle&) = delete; Is this different than DISALLOW_COPY_AND_ASSIGN(EntryHandle)? http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.h@145 PS10, Line 145: // Invoked whenever a cached entry reaches zero reference count, i.e. it was : // removed from the cache and there aren't any other references : // floating around. : std::unique_ptr<Cache::EvictionCallback> eviction_cb_; Can this be stack-allocated and const? http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.cc File src/kudu/master/table_locations_cache.cc: http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.cc@42 PS10, Line 42: nit: a couple too many spaces http://gerrit.cloudera.org:8080/#/c/15971/10/src/kudu/master/table_locations_cache.cc@95 PS10, Line 95: auto h(cache_->Insert(std::move(pending), eviction_cb_.get())); If this isn't being done under keys_by_table_id_lock_, isn't it possible that we Put() and immediately Remove(), but end up with the entry added into the cache, and nothing in keys_by_table_id_? I think that'd mean we're leaving memory on the table in the cache since we may never evict those handles. -- To view, visit http://gerrit.cloudera.org:8080/15971 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7d2a4771ddc455d92a1da00db91c555a21151a23 Gerrit-Change-Number: 15971 Gerrit-PatchSet: 10 Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Greg Solovyev <gsolov...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 08 Jul 2020 06:31:57 +0000 Gerrit-HasComments: Yes
