Will Berkeley has posted comments on this change. Change subject: KUDU-2044 Tombstoned tablets show up in /metrics ......................................................................
Patch Set 1: (7 comments) http://gerrit.cloudera.org:8080/#/c/7981/1//COMMIT_MSG Commit Message: PS1, Line 12: If a new copy of the tablet is placed on the server, the new : tablet's metric entity will replace the old one, if the old one's : hasn't been deregistered yet (steps toward deregistration occur only : when the entity publishes metrics e.g. on a call to /metrics). > So what happens if a new copy lands _before_ the old entity was deregistere The old entity is deregistered. This situation is what's tested in tablet_copy-itest, since nothing is calling /metrics to cause the retirement + deregistration. http://gerrit.cloudera.org:8080/#/c/7981/1/src/kudu/integration-tests/tablet_copy-itest.cc File src/kudu/integration-tests/tablet_copy-itest.cc: PS1, Line 79: METRIC_DECLARE_entity(tablet); : METRIC_DECLARE_counter(rows_inserted); > Nit: move this to the declarations below. Done Line 1540: > Would it be useful to also check at this point that GetInt64Metric fails, b It would be, but there's a small chance for flakiness since the entity may already have been republished (tablet recreated) after the itest::DeleteTablet call returns, depending on timing. It'd be unusual, but possible. I think it's ok to leave out since there's another test checking that the metric doesn't show up after unpublishing. The purpose of this test is to make sure the metrics reset if the tablet is revived. http://gerrit.cloudera.org:8080/#/c/7981/1/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: Line 304: metric_entity_->Depublish(); > Don't you have to check if this is non-null first? Doesn't look like it's g Done http://gerrit.cloudera.org:8080/#/c/7981/1/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: Line 2595: // Therefore, a third call will not see tablet metrics. > Maybe integrate this into the loop and check the value of 'i' to decide whe Done http://gerrit.cloudera.org:8080/#/c/7981/1/src/kudu/util/metrics.cc File src/kudu/util/metrics.cc: Line 483: UpdateReturnCopy(&entities_, id, e, nullptr); > Since you're not interested in the returned value, a simpler "entities_[id] Done http://gerrit.cloudera.org:8080/#/c/7981/1/src/kudu/util/metrics.h File src/kudu/util/metrics.h: PS1, Line 503: depublished > Nit: "unpublish" is more grammatically correct. Could you use that instead Done -- To view, visit http://gerrit.cloudera.org:8080/7981 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-HasComments: Yes
