Adar Dembo 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 deregistered? 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. Line 1540: Would it be useful to also check at this point that GetInt64Metric fails, because the entity has been unpublished? 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 guaranteed to be created. 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 whether to call STR_CONTAINS or STR_NOT_CONTAINS? 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] = e" should suffice. 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 of "depublish"? -- 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-HasComments: Yes
