You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Will Berkeley (Code Review)" <ge...@cloudera.org> on 2017/09/06 19:53:25 UTC

[kudu-CR] Add tablet state summary metrics 

Will Berkeley has uploaded a new change for review.

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

Change subject: Add tablet state summary metrics
......................................................................

Add tablet state summary metrics

This patch adds metrics for the number of tablets in each state
(i.e. the number of tablets for each value of TabletStatePB). To
avoid contention in the tablet manager, the frequency with which
the state numbers are computed is limited. Requests that come
too soon for an update will receive cached counts. The maximum
frequency of updates is controlled by a new flag.

Change-Id: I9fda2061d341586f0aa343787af59984a627080a
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
5 files changed, 293 insertions(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add tablet state summary metrics

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.

Change subject: Add tablet state summary metrics
......................................................................


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7980/7/src/kudu/tserver/ts_tablet_manager_metrics.h
File src/kudu/tserver/ts_tablet_manager_metrics.h:

Line 47:   int num_not_initialized_;
> nit: don't suffix public struct fields with _ per https://google.github.io/
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add tablet state summary metrics

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.

Change subject: Add tablet state summary metrics
......................................................................


Patch Set 2:

(4 comments)

I added a test and fixed a bug (plus the changes from your comments)

http://gerrit.cloudera.org:8080/#/c/7980/1/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

PS1, Line 1259:         // pass
              :        
> Got some tabs in here.
Done


PS1, Line 1267: int TSTabletManager::NumNotInitializedTablets() {
              :   return UpdateTabletStateSummaryAndReturnCount(tablet::NOT_INITIALIZED);
              : }
> Since this pattern is repeated a lot, perhaps you could change UpdateTablet
Done


http://gerrit.cloudera.org:8080/#/c/7980/1/src/kudu/tserver/ts_tablet_manager.h
File src/kudu/tserver/ts_tablet_manager.h:

Line 94:         return num_not_initialized;
> warning: missing username/bug in TODO [google-readability-todo]
git blame'd on todd


Line 308:   // Handle the case on startup where we find a tablet that is not in
> Nit: convention is to add the "Unlocked" suffix to functions that must be c
Superseded by later comment + fix.


-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add tablet state summary metrics

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: Add tablet state summary metrics
......................................................................


Patch Set 5:

(4 comments)

Looking pretty good, just a couple nits.

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

Line 1530:   workload.StopAndJoin();
Why not stop the workload on line 1523, as soon as we have the number of rows inserted that we want? The fact that we leave it running here made me think we were trying to test some concurrency condition here, but that's not the case.


PS5, Line 1544: Sleep for > 10ms
This comment seems out of date.


http://gerrit.cloudera.org:8080/#/c/7980/5/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

Line 115: DEFINE_int32(tablet_state_walk_min_period_ms, 1000,
What do you think about putting the metrics prototypes, callbacks, and registration into its own class with its own file? For example, a TsTabletMangerMetrics class in a ts_tablet_manager_metrics.cc file like TabletMetrics is structured. That could keep the bulk of the boilerplate out of this file (which is already large), except perhaps for the gflag and TSTabletManager::UpdateTabletStateSummaryAndReturnCount() function, which probably can't be easily factored out.


Line 1226:   MonoDelta period = MonoDelta::FromMilliseconds(FLAGS_tablet_state_walk_min_period_ms);
We can initialize this object before acquiring the lock


-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add tablet state summary metrics

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.

Change subject: Add tablet state summary metrics
......................................................................


Patch Set 7:

> IWYU gives very different (even contradictory) results on OS X v.
 > in the Jenkins build :(

Sorry for the inconvenience -- I need to iron out some quirks in IWYU for OS X.  As for now, I could suggest to build and run the IWYU verification at one of our Linux build servers: anyway, it's nice to do that since the code might compile at OS X but fail to compile at Linux.

-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] Add tablet state summary metrics

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Add tablet state summary metrics
......................................................................


Patch Set 1:

(5 comments)

Has anything materially changed since the split?

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

PS1, Line 79: METRIC_DECLARE_gauge_int32(tablets_num_running);
            : METRIC_DECLARE_gauge_int32(tablets_num_bootstrapping);
Nit: move this down to where the other metrics are declared?


PS1, Line 1545:   SleepFor(MonoDelta::FromMilliseconds(15));
              :   ASSERT_EQ(1, CountRunningTablets(cluster_->tablet_server(follower_index)));
Can you use ASSERT_EVENTUALLY rather than sleeping?

Below too.


http://gerrit.cloudera.org:8080/#/c/7980/1/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

PS1, Line 1259: 	// pass
              : 	break;
Got some tabs in here.


PS1, Line 1267:   std::lock_guard<rw_spinlock> lock(lock_);
              :   UpdateTabletStateSummary();
              :   return tablet_state_summary_.num_not_initialized;
Since this pattern is repeated a lot, perhaps you could change UpdateTabletStateSummary to accommodate it:

1. Have it acquire/release the lock, and
2. Have it return the appropriate counter value (specified via argument).


http://gerrit.cloudera.org:8080/#/c/7980/1/src/kudu/tserver/ts_tablet_manager.h
File src/kudu/tserver/ts_tablet_manager.h:

Line 308:   void UpdateTabletStateSummary();
Nit: convention is to add the "Unlocked" suffix to functions that must be called with a lock held.


-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes

[kudu-CR] Add tablet state summary metrics

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Add tablet state summary metrics
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7980/2/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

Line 1227:   if (last_walked_ + period > MonoTime::Now()) {
Nit: maybe refactor:

  if (last_walked_ + period < MonoTime::Now()) {
    // Our cached counts are too old, regenerate them.
    <walk map and update counts>
    last_walked_ = Now();
  }
  return CountForState(st);


http://gerrit.cloudera.org:8080/#/c/7980/2/src/kudu/tserver/ts_tablet_manager.h
File src/kudu/tserver/ts_tablet_manager.h:

Line 329:   // Update the TabletStateSummary for this tablet manager.
Update this.


-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add tablet state summary metrics

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Add tablet state summary metrics
......................................................................


Patch Set 3:

(2 comments)

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

Line 1555:   SleepFor(MonoDelta::FromMilliseconds(15));
Shouldn't need an explicit sleep anymore; let ASSERT_EVENTUALLY() take care of it.

Below too.


http://gerrit.cloudera.org:8080/#/c/7980/3/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

PS3, Line 1230: auto &
Nit: 'const auto&' is actually more GSG-compliant, since the '&' is part of the type.


-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add tablet state summary metrics

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.

Change subject: Add tablet state summary metrics
......................................................................


Patch Set 7:

No worries just don't want it to seem like I'm negligent and I don't bother to run IWYU before I post a new patch set.

-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] Add tablet state summary metrics

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: Add tablet state summary metrics
......................................................................

Add tablet state summary metrics

This patch adds metrics for the number of tablets in each state
(i.e. the number of tablets for each value of TabletStatePB). To
avoid contention in the tablet manager, the frequency with which
the state numbers are computed is limited. Requests that come
too soon for an update will receive cached counts. The maximum
frequency of updates is controlled by a new flag.

Change-Id: I9fda2061d341586f0aa343787af59984a627080a
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tserver/CMakeLists.txt
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
A src/kudu/tserver/ts_tablet_manager_metrics.cc
A src/kudu/tserver/ts_tablet_manager_metrics.h
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
8 files changed, 380 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/7980/9
-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add tablet state summary metrics

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: Add tablet state summary metrics
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7980/8/src/kudu/tserver/ts_tablet_manager_metrics.h
File src/kudu/tserver/ts_tablet_manager_metrics.h:

PS8, Line 56:   TSTabletManager* ts_tablet_manager;
            : 
            :   FunctionGaugeDetacher metric_detacher;
Don't shoot me, but shouldn't these two members be private? Aren't they never meant to be accessed by other classes?


-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add tablet state summary metrics

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: Add tablet state summary metrics
......................................................................

Add tablet state summary metrics

This patch adds metrics for the number of tablets in each state
(i.e. the number of tablets for each value of TabletStatePB). To
avoid contention in the tablet manager, the frequency with which
the state numbers are computed is limited. Requests that come
too soon for an update will receive cached counts. The maximum
frequency of updates is controlled by a new flag.

Change-Id: I9fda2061d341586f0aa343787af59984a627080a
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tserver/CMakeLists.txt
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
A src/kudu/tserver/ts_tablet_manager_metrics.cc
A src/kudu/tserver/ts_tablet_manager_metrics.h
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
8 files changed, 379 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/7980/8
-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add tablet state summary metrics

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: Add tablet state summary metrics
......................................................................

Add tablet state summary metrics

This patch adds metrics for the number of tablets in each state
(i.e. the number of tablets for each value of TabletStatePB). To
avoid contention in the tablet manager, the frequency with which
the state numbers are computed is limited. Requests that come
too soon for an update will receive cached counts. The maximum
frequency of updates is controlled by a new flag.

Change-Id: I9fda2061d341586f0aa343787af59984a627080a
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
5 files changed, 308 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add tablet state summary metrics

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: Add tablet state summary metrics
......................................................................

Add tablet state summary metrics

This patch adds metrics for the number of tablets in each state
(i.e. the number of tablets for each value of TabletStatePB). To
avoid contention in the tablet manager, the frequency with which
the state numbers are computed is limited. Requests that come
too soon for an update will receive cached counts. The maximum
frequency of updates is controlled by a new flag.

Change-Id: I9fda2061d341586f0aa343787af59984a627080a
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tserver/CMakeLists.txt
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
A src/kudu/tserver/ts_tablet_manager_metrics.cc
A src/kudu/tserver/ts_tablet_manager_metrics.h
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
8 files changed, 379 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/7980/7
-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add tablet state summary metrics

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: Add tablet state summary metrics
......................................................................


Patch Set 4: Code-Review+1

Please get an additional review from Mike and/or Todd.

-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] Add tablet state summary metrics

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged.

Change subject: Add tablet state summary metrics
......................................................................


Add tablet state summary metrics

This patch adds metrics for the number of tablets in each state
(i.e. the number of tablets for each value of TabletStatePB). To
avoid contention in the tablet manager, the frequency with which
the state numbers are computed is limited. Requests that come
too soon for an update will receive cached counts. The maximum
frequency of updates is controlled by a new flag.

Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Reviewed-on: http://gerrit.cloudera.org:8080/7980
Tested-by: Kudu Jenkins
Reviewed-by: Mike Percy <mp...@apache.org>
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tserver/CMakeLists.txt
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
A src/kudu/tserver/ts_tablet_manager_metrics.cc
A src/kudu/tserver/ts_tablet_manager_metrics.h
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
8 files changed, 380 insertions(+), 7 deletions(-)

Approvals:
  Mike Percy: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add tablet state summary metrics

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: Add tablet state summary metrics
......................................................................

Add tablet state summary metrics

This patch adds metrics for the number of tablets in each state
(i.e. the number of tablets for each value of TabletStatePB). To
avoid contention in the tablet manager, the frequency with which
the state numbers are computed is limited. Requests that come
too soon for an update will receive cached counts. The maximum
frequency of updates is controlled by a new flag.

Change-Id: I9fda2061d341586f0aa343787af59984a627080a
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tserver/CMakeLists.txt
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
A src/kudu/tserver/ts_tablet_manager_metrics.cc
A src/kudu/tserver/ts_tablet_manager_metrics.h
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
8 files changed, 379 insertions(+), 12 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add tablet state summary metrics

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: Add tablet state summary metrics
......................................................................

Add tablet state summary metrics

This patch adds metrics for the number of tablets in each state
(i.e. the number of tablets for each value of TabletStatePB). To
avoid contention in the tablet manager, the frequency with which
the state numbers are computed is limited. Requests that come
too soon for an update will receive cached counts. The maximum
frequency of updates is controlled by a new flag.

Change-Id: I9fda2061d341586f0aa343787af59984a627080a
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
5 files changed, 303 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot

[kudu-CR] Add tablet state summary metrics

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.

Change subject: Add tablet state summary metrics
......................................................................


Patch Set 5:

(4 comments)

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

Line 1530:   workload.StopAndJoin();
> Why not stop the workload on line 1523, as soon as we have the number of ro
Done


PS5, Line 1544: Sleep for > 10ms
> This comment seems out of date.
Done


http://gerrit.cloudera.org:8080/#/c/7980/5/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

Line 115: DEFINE_int32(tablet_state_walk_min_period_ms, 1000,
> What do you think about putting the metrics prototypes, callbacks, and regi
Done. It's a wee bit awkward since the metrics struct and the tablet manager both need to reference each other because the (refactored) UpdateTabletStateSummaryAndReturnCount function most naturally lives in the tablet manager, but it does cut down the boilerplate in the tablet manager code.


Line 1226:   MonoDelta period = MonoDelta::FromMilliseconds(FLAGS_tablet_state_walk_min_period_ms);
> We can initialize this object before acquiring the lock
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add tablet state summary metrics

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,

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

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

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

Change subject: Add tablet state summary metrics
......................................................................

Add tablet state summary metrics

This patch adds metrics for the number of tablets in each state
(i.e. the number of tablets for each value of TabletStatePB). To
avoid contention in the tablet manager, the frequency with which
the state numbers are computed is limited. Requests that come
too soon for an update will receive cached counts. The maximum
frequency of updates is controlled by a new flag.

Change-Id: I9fda2061d341586f0aa343787af59984a627080a
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
5 files changed, 308 insertions(+), 5 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] Add tablet state summary metrics

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.

Change subject: Add tablet state summary metrics
......................................................................


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7980/8/src/kudu/tserver/ts_tablet_manager_metrics.h
File src/kudu/tserver/ts_tablet_manager_metrics.h:

PS8, Line 56:   TSTabletManager* ts_tablet_manager;
            : 
            :   FunctionGaugeDetacher metric_detacher;
> Don't shoot me, but shouldn't these two members be private? Aren't they nev
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add tablet state summary metrics

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.

Change subject: Add tablet state summary metrics
......................................................................


Patch Set 3:

(5 comments)

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

PS1, Line 1545:   ASSERT_EVENTUALLY([&] {
              :     ASSERT_EQ(1, CountRunningTablets(cluster_->tablet_server(follower_index))
> Can you use ASSERT_EVENTUALLY rather than sleeping?
Done


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

Line 1555:   SleepFor(MonoDelta::FromMilliseconds(15));
> Shouldn't need an explicit sleep anymore; let ASSERT_EVENTUALLY() take care
:(


http://gerrit.cloudera.org:8080/#/c/7980/2/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

Line 1227:   if (last_walked_ + period < MonoTime::Now()) {
> Nit: maybe refactor:
Done


http://gerrit.cloudera.org:8080/#/c/7980/3/src/kudu/tserver/ts_tablet_manager.cc
File src/kudu/tserver/ts_tablet_manager.cc:

PS3, Line 1230: auto &
> Nit: 'const auto&' is actually more GSG-compliant, since the '&' is part of
I plead innocent. My IDE moves *'s and &'s when I'm not looking. I did go fix the setting for it, so shouldn't happen anymore.


http://gerrit.cloudera.org:8080/#/c/7980/2/src/kudu/tserver/ts_tablet_manager.h
File src/kudu/tserver/ts_tablet_manager.h:

Line 329:   // Update the TabletStateSummary for this tablet manager and return the
> Update this.
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add tablet state summary metrics

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: Add tablet state summary metrics
......................................................................


Patch Set 7:

(1 comment)

looks good, just one nit on something that changed since the last review

http://gerrit.cloudera.org:8080/#/c/7980/7/src/kudu/tserver/ts_tablet_manager_metrics.h
File src/kudu/tserver/ts_tablet_manager_metrics.h:

Line 47:   int num_not_initialized_;
nit: don't suffix public struct fields with _ per https://google.github.io/styleguide/cppguide.html#Variable_Names


-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] Add tablet state summary metrics

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change.

Change subject: Add tablet state summary metrics
......................................................................


Patch Set 7:

IWYU gives very different (even contradictory) results on OS X v. in the Jenkins build :(

-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] Add tablet state summary metrics

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.

Change subject: Add tablet state summary metrics
......................................................................


Patch Set 9: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/7980
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] Add tablet state summary metrics

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: Add tablet state summary metrics
......................................................................

Add tablet state summary metrics

This patch adds metrics for the number of tablets in each state
(i.e. the number of tablets for each value of TabletStatePB). To
avoid contention in the tablet manager, the frequency with which
the state numbers are computed is limited. Requests that come
too soon for an update will receive cached counts. The maximum
frequency of updates is controlled by a new flag.

Change-Id: I9fda2061d341586f0aa343787af59984a627080a
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tserver/ts_tablet_manager.cc
M src/kudu/tserver/ts_tablet_manager.h
M src/kudu/util/metrics.cc
M src/kudu/util/metrics.h
5 files changed, 306 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9fda2061d341586f0aa343787af59984a627080a
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>