Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/7981 )
Change subject: KUDU-2044 Tombstoned tablets show up in /metrics ...................................................................... Patch Set 6: (6 comments) LGTM, just a couple nits http://gerrit.cloudera.org:8080/#/c/7981/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/7981/6//COMMIT_MSG@20 PS6, Line 20: tombstoned's tablets tombstoned tablet's http://gerrit.cloudera.org:8080/#/c/7981/6/src/kudu/integration-tests/tablet_copy-itest.cc File src/kudu/integration-tests/tablet_copy-itest.cc: http://gerrit.cloudera.org:8080/#/c/7981/6/src/kudu/integration-tests/tablet_copy-itest.cc@1726 PS6, Line 1726: // Find the leader so we can tombstone a follower, which makes the test faster and more focused. : int leader_index; : int follower_index; : TServerDetails* leader_ts; : TServerDetails* follower_ts; : ASSERT_OK(FindTabletLeader(ts_map_, tablet_id, kTimeout, &leader_ts)); : leader_index = cluster_->tablet_server_index_by_uuid(leader_ts->uuid()); : follower_index = (leader_index + 1) % cluster_->num_tablet_servers(); : follower_ts = ts_map_[cluster_->tablet_server(follower_index)->uuid()]; : : // Make sure the metrics reflect that some rows have been inserted. : ASSERT_GT(CountRowsInserted(cluster_->tablet_server(follower_index)), 0); : : // Tombstone the tablet on the follower. : ASSERT_OK(itest::DeleteTablet(follower_ts, tablet_id, : TABLET_DATA_TOMBSTONED, : boost::none, kTimeout)); This bit should be wrapped in ASSERT_EVENTUALLY() { ... } in order to ensure that the test doesn't get flaky due to typical latency volatility on the test machines. http://gerrit.cloudera.org:8080/#/c/7981/6/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/7981/6/src/kudu/tserver/tablet_server-test.cc@2583 PS6, Line 2583: It takes three calls Thanks for the explanation. Can you please add: "at the time of writing, based on the policy in ClassFoo" or "foo.cc", or something like that? http://gerrit.cloudera.org:8080/#/c/7981/6/src/kudu/tserver/tablet_server-test.cc@2591 PS6, Line 2591: mini nit: indent this another 4 spaces to line up with the parameter on the above line http://gerrit.cloudera.org:8080/#/c/7981/6/src/kudu/tserver/tablet_server-test.cc@2592 PS6, Line 2592: &buf nit: the indentation of this line seems random http://gerrit.cloudera.org:8080/#/c/7981/6/src/kudu/util/metrics.h File src/kudu/util/metrics.h: http://gerrit.cloudera.org:8080/#/c/7981/6/src/kudu/util/metrics.h@544 PS6, Line 544: bool unpublished_; Sometimes thinking in negatives can be confusing. How about flipping this and the corresponding getter method to "published" ? -- To view, visit http://gerrit.cloudera.org:8080/7981 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3 Gerrit-Change-Number: 7981 Gerrit-PatchSet: 6 Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-Comment-Date: Mon, 02 Oct 2017 19:56:33 +0000 Gerrit-HasComments: Yes
