Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12634 )
Change subject: [master] introduce cache for location mapping assignments ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/12634/1/src/kudu/master/location_cache-test.cc File src/kudu/master/location_cache-test.cc: http://gerrit.cloudera.org:8080/#/c/12634/1/src/kudu/master/location_cache-test.cc@172 PS1, Line 172: : { : vector<thread> threads; : threads.reserve(kNumThreads); : for (auto idx = 0; idx < kNumThreads; ++idx) { : threads.emplace_back([&cache, &kRefLocation, idx]() { : string location; : auto s = cache.GetLocation(Substitute("key_$0", idx), &location); : CHECK(s.ok()) << s.ToString(); : CHECK_EQ(kRefLocation, location); : }); : } : for (auto& t : threads) { : t.join(); : } : } > This is duplicated from above; consider using a for loop to just run it twi Done http://gerrit.cloudera.org:8080/#/c/12634/1/src/kudu/master/location_cache.h File src/kudu/master/location_cache.h: http://gerrit.cloudera.org:8080/#/c/12634/1/src/kudu/master/location_cache.h@31 PS1, Line 31: // Get the location for the specified key. > nit: describe the cache and its properties? Unlimited size, never invalidat Done http://gerrit.cloudera.org:8080/#/c/12634/1/src/kudu/master/location_cache.h@43 PS1, Line 43: 'cmd'. > Might be missing something, but why not use location_mapping_cmd_? This was the original function used to perform location mapping and it doesn't have dependency on any piece of state in this class. Keeping it as is seems cleaner, IMO. http://gerrit.cloudera.org:8080/#/c/12634/1/src/kudu/master/location_cache.h@64 PS1, Line 64: queries_ > nit: maybe call this location_mapping_cache_misses_ then? Ah, the comment is outdated, sorry: in one version there was just a counter for number of runs of the command. Fixed. -- To view, visit http://gerrit.cloudera.org:8080/12634 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icb5c436c9433acd87c44c4d81982420f33ebb4a4 Gerrit-Change-Number: 12634 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Thu, 28 Feb 2019 16:21:55 +0000 Gerrit-HasComments: Yes
