helifu 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: (7 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::functio Done 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. Done 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: Thanks, Adar! :) 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()); > Hmm, why do we normalize the table name on L354 then? Ah, gotcha! It does need to be normalized when the HMS integration switches from off to on. http://gerrit.cloudera.org:8080/#/c/13426/20/src/kudu/master/catalog_manager.cc@1785 PS20, Line 1785: metadata->set_create_timestamp(time(nullptr)); > Likewise, normalized table name? The table name has been normalized above Line1775, so it's not necessary to normalize it again. By the way, I have a question about Line1775. If we normalize the table name and store it in the metadata when HMS integration is enabled, how can we restore the original table name when HMS integration is disabled? 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. Done 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: Done -- 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: Tue, 23 Jul 2019 12:02:03 +0000 Gerrit-HasComments: Yes
