Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13426 )
Change subject: KUDU-2797 p2: the master aggregates tablet statistics ...................................................................... Patch Set 24: (6 comments) http://gerrit.cloudera.org:8080/#/c/13426/24/src/kudu/integration-tests/ts_tablet_manager-itest.cc File src/kudu/integration-tests/ts_tablet_manager-itest.cc: http://gerrit.cloudera.org:8080/#/c/13426/24/src/kudu/integration-tests/ts_tablet_manager-itest.cc@589 PS24, Line 589: const auto GetLeaderMaster = [&] (Master** master) { I wonder if this would be more useful as a lambda that took an std::function as an argument and: 1. Acquired/verified the ScopedLeaderSharedLock. 2. Ran the std::function 3. Released the ScopedLeaderSharedLock. http://gerrit.cloudera.org:8080/#/c/13426/24/src/kudu/integration-tests/ts_tablet_manager-itest.cc@631 PS24, Line 631: ASSERT_EQ(kNumMasters, cluster_->num_masters()); : ASSERT_EQ(kNumTservers, cluster_->num_tablet_servers()); Not necessary. http://gerrit.cloudera.org:8080/#/c/13426/24/src/kudu/integration-tests/ts_tablet_manager-itest.cc@678 PS24, Line 678: ASSERT_OK(con->EmulateElection()); You sure this is safe? The function comment says: // This is NOT safe to use in a distributed configuration with failure detection // enabled, as it could result in a split-brain scenario. How about using LeaderStepDown instead? http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.cc@370 PS20, Line 370: table->RegisterMetrics(catalog_manager_->master_->metric_registry(), metadata.name()); > I think the table name in the metadata should be a normalized string alread Hmm, why do we normalize the table name on L354 then? http://gerrit.cloudera.org:8080/#/c/13426/24/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/13426/24/src/kudu/master/catalog_manager.cc@5515 PS24, Line 5515: std:: Don't need. http://gerrit.cloudera.org:8080/#/c/13426/24/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: http://gerrit.cloudera.org:8080/#/c/13426/24/src/kudu/tablet/tablet_replica.cc@824 PS24, Line 824: if (RUNNING == state()) { To avoid deep nesting, invert this: if (RUNNING != state()) { return; } ... -- To view, visit http://gerrit.cloudera.org:8080/13426 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I74406ab7cca7c22fda455c328b8ee9989a6b2d99 Gerrit-Change-Number: 13426 Gerrit-PatchSet: 24 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Mon, 22 Jul 2019 22:51:46 +0000 Gerrit-HasComments: Yes
