Alexey Serbin 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? Yes, I think it does. I think this piece is a copy-paste byproduct :) 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? Done 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)? I guess it's supposed to be the same, but DISALLOW_COPY_AND_ASSIGN() for some reason has operator=() as void. 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? I guess it might be such a thing, but there is an issue with the signature of the EvictionCallback class: it requires a pointer to the outer cache class. 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 Done 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 th Good catch! Fixed. -- 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: Thu, 09 Jul 2020 01:21:28 +0000 Gerrit-HasComments: Yes