Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12619 )
Change subject: [master] cache assigned locations ...................................................................... Patch Set 2: (8 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? 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 for testing: 1. Happy path. 2. Multiple concurrent GetLocation calls (to test synchronization). 3. Failed mapping command. 4. Metrics are incremented as they should be. By and large I think this coverage is spread out amongst existing tests, but it's always nice to have a very targeted unit test for a new module, to provide a really fast dev-test loop while working on that module. 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@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. 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 TSManager can both reference LocationCache. Given that the Master's lifetime is guaranteed to outlive that of the TSManager, why can't the TSManager store a raw pointer to the LocationCache? Doesn't seem like shared ownership is warranted in this simple case. 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 provided? Alternatively, add another constructor for use in tests that doesn't require a metric entity. 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. 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? http://gerrit.cloudera.org:8080/#/c/12619/2/src/kudu/tools/ksck_remote-test.cc@576 PS2, Line 576: err_stream_.str(); No effect? -- 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 00:59:25 +0000 Gerrit-HasComments: Yes
