Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12619 )
Change subject: [master] cache assigned locations ...................................................................... Patch Set 2: (11 comments) http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/client/client-test.cc@5995 PS2, Line 5995: const auto queries_before = counter_queries->value(); : const auto hits_before = counter_hits->value(); > Should we ASSERT that queries_before is 1 and hits_before is 0? Done http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/master/location_cache.h File src/kudu/master/location_cache.h: http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/master/location_cache.h@32 PS2, Line 32: class LocationCache { > Would be good to have a dedicated unit test for LocationCache. Candidates f I totally agree -- having a unit test helps. Is it OK if I add it in a separate changelist? http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/master/location_cache.cc File src/kudu/master/location_cache.cc: http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/master/location_cache.cc@76 PS2, Line 76: LocationCache::LocationCache(const string& location_mapping_cmd, > warning: pass by value and use std::move [modernize-pass-by-value] Done http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/master/location_cache.cc@105 PS2, Line 105: Status s = GetLocationFromLocationMappingCmd( : location_mapping_cmd_, key, &value); > May want to add a TRACE to capture the amount of time this takes to run. Done http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/master/location_cache.cc@128 PS2, Line 128: Status s = Subprocess::Call(argv, /*stdin=*/"", &location_temp, &stderr); > warning: argument name 'stdin' in comment does not match parameter name 'st Done http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/master/master.h File src/kudu/master/master.h: http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/master/master.h@158 PS2, Line 158: // A simplistic cache to track already assigned locations. : std::shared_ptr<LocationCache> location_cache_; > Looks like the only reason for shared ownership is so that Master and TSMan Yep, this seems to be a good idea. I was not sure about the ownership relationship given so many gscoped_ptr in TSManager, but TSManager appears to be owned only by master itself. http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/master/ts_descriptor-test.cc File src/kudu/master/ts_descriptor-test.cc: http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/master/ts_descriptor-test.cc@96 PS2, Line 96: LocationCache cache(location_cmd, nullptr); > Could you annotate the nullptr here and below to indicate what's not being I decided to set the metric_entity parameter as nullptr by default. http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/master/ts_manager.h File src/kudu/master/ts_manager.h: http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/master/ts_manager.h@52 PS2, Line 52: explicit > No need for 'explicit' when it's not a single-arg constructor. Done http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/master/ts_manager.cc File src/kudu/master/ts_manager.cc: http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/master/ts_manager.cc@52 PS2, Line 52: TSManager::TSManager(const shared_ptr<LocationCache>& location_cache, > warning: pass by value and use std::move [modernize-pass-by-value] Done http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/tools/ksck_remote-test.cc File src/kudu/tools/ksck_remote-test.cc: http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/tools/ksck_remote-test.cc@541 PS2, Line 541: err_stream_.str(); > Does this have any effect? Indeed, this seems to be obsolete -- I removed this piece. http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/tools/ksck_remote-test.cc@576 PS2, Line 576: err_stream_.str(); > No effect? whoops, should have been err_stream_.clear() or something, but now it's obsolete. -- To view, visit http://gerrit.cloudera.org:8080/12619 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I12c8952c43a8ad352acd46c8006824b2ad9d1204 Gerrit-Change-Number: 12619 Gerrit-PatchSet: 2 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: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Thu, 28 Feb 2019 01:58:53 +0000 Gerrit-HasComments: Yes
