[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7981 )

Change subject: KUDU-2044 Tombstoned tablets show up in /metrics
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7981/7/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:

http://gerrit.cloudera.org:8080/#/c/7981/7/src/kudu/util/metrics.h@511
PS7, Line 511:   bool published() {
> nit: maybe move the comment describing the var here?
Because the GSG guideline suggests a method named like this just returns the 
value of an instance variable, and because published_ needs a doc about its 
lock anyway, I'm going to keep the comment on the var.



--
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: 7
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Tue, 17 Oct 2017 14:54:46 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Will Berkeley (Code Review)]

Will Berkeley has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/7981 )

Change subject: KUDU-2044 Tombstoned tablets show up in /metrics
..

KUDU-2044 Tombstoned tablets show up in /metrics

This patch stops tombstoned tablets from showing up in /metrics. When a
tablet replica is shut down, the tablet's metric entity is marked as
unpublished. All its child metrics will be marked for retirement, then
retired after the retirement interval has passed. Then the entity
itself is unregistered, i.e. removed from the metric registry's map of
entities. If a new replica of the same tablet is added to the server,
it will create a new entity that will be registered with the
metric_registry, either as a new insertion or overwriting the old
replica's entity if the entity had been unpublished but not yet
unregistered.

The tombstoned tablet's metric entity is not destroyed when it's
deregistered, since there are still refs to it and its metrics in the
TabletReplica class, Tablet class, and many other classes associated
with a TabletReplica. The entity will be destroyed when the
TabletReplica is deleted (since it either contains or holds final
references to all the other classes), which happens if the tablet is
replaced or deleted. While it's not ideal to hold the memory for the
entity when it's no longer used, the reason this occurs is because the
TabletReplica instance mostly stays alive. Releasing all the metric
references would require specifically dropping refs to those metrics in
all the surviving subcomponents of a TabletReplica instance that has
been shut down; I think this problem would be better solved by more
completely cleaning up a shut down TabletReplica instance, but that's a
much larger scope than suppressing the metrics.

Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
Reviewed-on: http://gerrit.cloudera.org:8080/7981
Tested-by: Kudu Jenkins
Reviewed-by: David Ribeiro Alves 
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
5 files changed, 146 insertions(+), 11 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  David Ribeiro Alves: Looks good to me, approved

--
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: merged
Gerrit-Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
Gerrit-Change-Number: 7981
Gerrit-PatchSet: 8
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7981 )

Change subject: KUDU-2044 Tombstoned tablets show up in /metrics
..


Patch Set 7: Code-Review+2

(1 comment)

xxs nit fell free to fix re+2 and push or ignore (and push)

http://gerrit.cloudera.org:8080/#/c/7981/7/src/kudu/util/metrics.h
File src/kudu/util/metrics.h:

http://gerrit.cloudera.org:8080/#/c/7981/7/src/kudu/util/metrics.h@511
PS7, Line 511:   bool published() {
nit: maybe move the comment describing the var here?



--
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: 7
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Tue, 17 Oct 2017 01:33:43 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Will Berkeley (Code Review)]

Hello Tidy Bot, Mike Percy, Kudu Jenkins, Adar Dembo,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/7981

to look at the new patch set (#7).

Change subject: KUDU-2044 Tombstoned tablets show up in /metrics
..

KUDU-2044 Tombstoned tablets show up in /metrics

This patch stops tombstoned tablets from showing up in /metrics. When a
tablet replica is shut down, the tablet's metric entity is marked as
unpublished. All its child metrics will be marked for retirement, then
retired after the retirement interval has passed. Then the entity
itself is unregistered, i.e. removed from the metric registry's map of
entities. If a new replica of the same tablet is added to the server,
it will create a new entity that will be registered with the
metric_registry, either as a new insertion or overwriting the old
replica's entity if the entity had been unpublished but not yet
unregistered.

The tombstoned tablet's metric entity is not destroyed when it's
deregistered, since there are still refs to it and its metrics in the
TabletReplica class, Tablet class, and many other classes associated
with a TabletReplica. The entity will be destroyed when the
TabletReplica is deleted (since it either contains or holds final
references to all the other classes), which happens if the tablet is
replaced or deleted. While it's not ideal to hold the memory for the
entity when it's no longer used, the reason this occurs is because the
TabletReplica instance mostly stays alive. Releasing all the metric
references would require specifically dropping refs to those metrics in
all the surviving subcomponents of a TabletReplica instance that has
been shut down; I think this problem would be better solved by more
completely cleaning up a shut down TabletReplica instance, but that's a
much larger scope than suppressing the metrics.

Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
5 files changed, 146 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/7981/7
--
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: newpatchset
Gerrit-Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
Gerrit-Change-Number: 7981
Gerrit-PatchSet: 7
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Will Berkeley (Code Review)]

Will Berkeley 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)

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
Done


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, 
_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  ensu
Done


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, ba
Done


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 abov
Done


http://gerrit.cloudera.org:8080/#/c/7981/6/src/kudu/tserver/tablet_server-test.cc@2592
PS6, Line 2592: 
> nit: the indentation of this line seems random
It's the second argument to FetchURL, so it's indented to line up with the 
first argument (strings::Substitute(...))


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 a
Done



--
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Thu, 05 Oct 2017 17:52:27 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Mike Percy (Code Review)]

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, 
_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: 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-Comment-Date: Mon, 02 Oct 2017 19:56:33 +
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Will Berkeley (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7981

to look at the new patch set (#6).

Change subject: KUDU-2044 Tombstoned tablets show up in /metrics
..

KUDU-2044 Tombstoned tablets show up in /metrics

This patch stops tombstoned tablets from showing up in /metrics. When a
tablet replica is shut down, the tablet's metric entity is marked as
unpublished. All its child metrics will be marked for retirement, then
retired after the retirement interval has passed. Then the entity
itself is unregistered, i.e. removed from the metric registry's map of
entities. If a new replica of the same tablet is added to the server,
it will create a new entity that will be registered with the
metric_registry, either as a new insertion or overwriting the old
replica's entity if the entity had been unpublished but not yet
unregistered.

The tombstoned's tablets metric entity is not destroyed when it's
deregistered, since there are still refs to it and its metrics in the
TabletReplica class, Tablet class, and many other classes associated
with a TabletReplica. The entity will be destroyed when the
TabletReplica is deleted (since it either contains or holds final
references to all the other classes), which happens if the tablet is
replaced or deleted. While it's not ideal to hold the memory for the
entity when it's no longer used, the reason this occurs is because the
TabletReplica instance mostly stays alive. Releasing all the metric
references would require specifically dropping refs to those metrics in
all the surviving subcomponents of a TabletReplica instance that has
been shut down; I think this problem would be better solved by more
completely cleaning up a shut down TabletReplica instance, but that's a
much larger scope than suppressing the metrics.

Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
5 files changed, 143 insertions(+), 12 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/7981/6
-- 
To view, visit http://gerrit.cloudera.org:8080/7981
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Will Berkeley (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7981

to look at the new patch set (#5).

Change subject: KUDU-2044 Tombstoned tablets show up in /metrics
..

KUDU-2044 Tombstoned tablets show up in /metrics

This patch stops tombstoned tablets from showing up in /metrics. When a
tablet replica is shut down, the tablet's metric entity is marked as
unpublished. All its child metrics will be marked for retirement, then
retired after the retirement interval has passed. Then the entity
itself is unregistered, i.e. removed from the metric registry's map of
entities. If a new replica of the same tablet is added to the server,
it will create a new entity that will be registered with the
metric_registry, either as a new insertion or overwriting the old
replica's entity if the entity had been unpublished but not yet
unregistered.

The tombstoned's tablets metric entity is not destroyed when it's
deregistered, since there are still refs to it and its metrics in the
TabletReplica class, Tablet class, and many other classes associated
with a TabletReplica. The entity will be destroyed when the
TabletReplica is deleted (since it either contains or holds final
references to all the other classes), which happens if the tablet is
replaced or deleted. While it's not ideal to hold the memory for the
entity when it's no longer used, the reason this occurs is because the
TabletReplica instance mostly stays alive. Releasing all the metric
references would require specifically dropping refs to those metrics in
all the surviving subcomponents of a TabletReplica instance that has
been shut down; I think this problem would be better solved by more
completely cleaning up a shut down TabletReplica instance, but that's a
much larger scope than suppressing the metrics.

Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
5 files changed, 144 insertions(+), 13 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/7981/5
-- 
To view, visit http://gerrit.cloudera.org:8080/7981
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

Change subject: KUDU-2044 Tombstoned tablets show up in /metrics
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7981/3//COMMIT_MSG
Commit Message:

PS3, Line 10: it
> is
Done


http://gerrit.cloudera.org:8080/#/c/7981/3/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

PS3, Line 2582: do
> go?
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Will Berkeley (Code Review)]

Hello Adar Dembo, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7981

to look at the new patch set (#4).

Change subject: KUDU-2044 Tombstoned tablets show up in /metrics
..

KUDU-2044 Tombstoned tablets show up in /metrics

This patch stops tombstoned tablets from showing up in /metrics. When a
tablet replica is shut down, the tablet's metric entity is marked as
unpublished. All its child metrics will be marked for retirement, then
retired after the retirement interval has passed. Then the entity
itself is unregistered, i.e. removed from the metric registry's map of
entities. If a new replica of the same tablet is added to the server,
it will create a new entity that will be registered with the
metric_registry, either as a new insertion or overwriting the old
replica's entity if the entity had been unpublished but not yet
unregistered.

The tombstoned's tablets metric entity is not destroyed when it's
deregistered, since there are still refs to it and its metrics in the
TabletReplica class, Tablet class, and many other classes associated
with a TabletReplica. The entity will be destroyed when the
TabletReplica is deleted (since it either contains or holds final
references to all the other classes), which happens if the tablet is
replaced or deleted. While it's not ideal to hold the memory for the
entity when it's no longer used, the reason this occurs is because the
TabletReplica instance mostly stays alive. Releasing all the metric
references would require specifically dropping refs to those metrics in
all the surviving subcomponents of a TabletReplica instance that has
been shut down; I think this problem would be better solved by more
completely cleaning up a shut down TabletReplica instance, but that's a
much larger scope than suppressing the metrics.

Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
5 files changed, 144 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/7981/4
-- 
To view, visit http://gerrit.cloudera.org:8080/7981
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-2044 Tombstoned tablets show up in /metrics
..


Patch Set 3: Code-Review+1

(2 comments)

Please solicit reviews from Todd and Mike.

http://gerrit.cloudera.org:8080/#/c/7981/3//COMMIT_MSG
Commit Message:

PS3, Line 10: it
is


http://gerrit.cloudera.org:8080/#/c/7981/3/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

PS3, Line 2582: do
go?


-- 
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: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7981

to look at the new patch set (#3).

Change subject: KUDU-2044 Tombstoned tablets show up in /metrics
..

KUDU-2044 Tombstoned tablets show up in /metrics

This patch stops tombstoned tablets from showing up in /metrics. When a
tablet replica is shut down, the tablet's metric entity it marked as
unpublished. All its child metrics will be marked for retirement, then
retired after the retirement interval has passed. Then the entity
itself is unregistered, i.e. removed from the metric registry's map of
entities. If a new replica of the same tablet is added to the server,
it will create a new entity that will be registered with the
metric_registry, either as a new insertion or overwriting the old
replica's entity if the entity had been unpublished but not yet
unregistered.

The tombstoned's tablets metric entity is not destroyed when it's
deregistered, since there are still refs to it and its metrics in the
TabletReplica class, Tablet class, and many other classes associated
with a TabletReplica. The entity will be destroyed when the
TabletReplica is deleted (since it either contains or holds final
references to all the other classes), which happens if the tablet is
replaced or deleted. While it's not ideal to hold the memory for the
entity when it's no longer used, the reason this occurs is because the
TabletReplica instance mostly stays alive. Releasing all the metric
references would require specifically dropping refs to those metrics in
all the surviving subcomponents of a TabletReplica instance that has
been shut down; I think this problem would be better solved by more
completely cleaning up a shut down TabletReplica instance, but that's a
much larger scope than suppressing the metrics.

Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
5 files changed, 144 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/7981/3
-- 
To view, visit http://gerrit.cloudera.org:8080/7981
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

Change subject: KUDU-2044 Tombstoned tablets show up in /metrics
..


Patch Set 1:

(3 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).
> I'm still finding this sentence confusing. The old entity is replaced if it
Will reword. I think the confusion here is that deregister != unpublish. 
Unpublishing just marks the entity for eventual deregistration. Deregistration 
is simply the metric registry erasing the entity from its map of entities. 
Metrics ultimately stop showing up only when the entity is deregistered, not 
merely when it is unpublished.


http://gerrit.cloudera.org:8080/#/c/7981/2/src/kudu/integration-tests/tablet_copy-itest.cc
File src/kudu/integration-tests/tablet_copy-itest.cc:

Line 111: using std::vector;
> warning: redundant 'METRIC_ENTITY_server' declaration [readability-redundan
Done


http://gerrit.cloudera.org:8080/#/c/7981/2/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

PS2, Line 2582:   // The next two calls to /jsonmetricz should show tablet 
metrics:
  :   // 1. The first call causes the registry to discover the 
tablet's metric entity
  :   // has been de-published and marks it for retirement after 
the expiration period.
  :   // 2. The second call causes the entity to retire all its 
metrics, since we've
  :   // passed the retirement period. Then, the registry 
de-registers the entity.
  :   EasyCurl c;
> Nit: since you have a 2-item list already set up, I think formatting the th
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: KUDU-2044 Tombstoned tablets show up in /metrics
..


Patch Set 2:

(2 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).
> The old entity is deregistered. This situation is what's tested in tablet_c
I'm still finding this sentence confusing. The old entity is replaced if it 
_was not_ deregistered? And if it was already deregistered, then it's 
deregistered now, and replaced?

Could you maybe reword this to clarify exactly what's going on?


http://gerrit.cloudera.org:8080/#/c/7981/2/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:

PS2, Line 2582:   // The first two calls to /jsonmetricz should show tablet 
metrics:
  :   // 1. The first call causes the registry to discover the 
tablet's metric entity
  :   // has been unpublished and marks it for retirement after the 
expiration period.
  :   // 2. The second call causes the entity to retire all its 
metrics, since we've
  :   // passed the retirement period. Then, the registry 
de-registers the entity.
  :   // Therefore, a third call will not see tablet metrics.
Nit: since you have a 2-item list already set up, I think formatting the third 
call as the third item would be more readable. Something like:

  // We make three calls to /jsonmetrics.
  // 1. Return the metric, but mark it for retirement.
  // 2. Return it, but also retire it.
  // 3. Don't return it because it's been retired.


-- 
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7981

to look at the new patch set (#2).

Change subject: KUDU-2044 Tombstoned tablets show up in /metrics
..

KUDU-2044 Tombstoned tablets show up in /metrics

This patch stops tombstoned tablets from showing up in /metrics. It
marks the tablet metric entity as unpublished, which causes all of
its metrics to be retired and causes the metric registry to de-register
it. 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).

The tombstoned's tablets metric entity is not destroyed when it's
deregistered, since there are still refs to it and its metrics in the
TabletReplica class, Tablet class, and many other classes associated
with a TabletReplica. The entity will be destroyed when the
TabletReplica is deleted (since it either contains or holds final
references to all the other classes), which happens if the tablet is
replaced or deleted. While it's not ideal to hold the memory for the
entity when it's no longer used, the reason this occurs is because the
TabletReplica instance mostly stays alive. Releasing all the metric
references would require specifically dropping refs to those metrics in
all the surviving subcomponents of a TabletReplica instance that has
been shut down; I think this problem would be better solved by more
completely cleaning up a shut down TabletReplica instance, but that's a
much larger scope than suppressing the metrics.

Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
5 files changed, 146 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/7981/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7981
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Will Berkeley (Code Review)]

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(_, 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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Adar Dembo (Code Review)]

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(_, 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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2044 Tombstoned tablets show up in /metrics

[Will Berkeley (Code Review)]

Will Berkeley has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/7981

Change subject: KUDU-2044 Tombstoned tablets show up in /metrics
..

KUDU-2044 Tombstoned tablets show up in /metrics

This patch stops tombstoned tablets from showing up in /metrics. It
marks the tablet metric entity as unpublished, which causes all of
its metrics to be retired and causes the metric registry to de-register
it. 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).

The tombstoned's tablets metric entity is not destroyed when it's
deregistered, since there are still refs to it and its metrics in the
TabletReplica class, Tablet class, and many other classes associated
with a TabletReplica. The entity will be destroyed when the
TabletReplica is deleted (since it either contains or holds final
references to all the other classes), which happens if the tablet is
replaced or deleted. While it's not ideal to hold the memory for the
entity when it's no longer used, the reason this occurs is because the
TabletReplica instance mostly stays alive. Releasing all the metric
references would require specifically dropping refs to those metrics in
all the surviving subcomponents of a TabletReplica instance that has
been shut down; I think this problem would be better solved by more
completely cleaning up a shut down TabletReplica instance, but that's a
much larger scope than suppressing the metrics.

Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet.cc
M src/kudu/tserver/tablet_server-test.cc
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
5 files changed, 145 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/81/7981/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7981
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I21b52262203410ded1e514502b59a79be12f8fb3
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley