Will Berkeley has posted comments on this change.

Change subject: KUDU-1755 Part 2: Improve tablet on-disk size metric
......................................................................


Patch Set 17:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/6968/17/src/kudu/consensus/log.cc
File src/kudu/consensus/log.cc:

PS17, Line 909: std::lock_guard
> Does this need to be lock_guard? Could it be lock_shared<rw_spinlock> l(sta
I think percpu_rwlock doesn't support shared_lock.


http://gerrit.cloudera.org:8080/#/c/6968/17/src/kudu/tablet/diskrowset.cc
File src/kudu/tablet/diskrowset.cc:

PS17, Line 736: OnDiskDataSize()
> nit: may be worth commenting in the API declaration that this does not incl
Added (no deltas) for clarity. The explanatory comment above the declarations, 
when matched with the comments on the declarations, does make it clear exactly 
what a given *OnDiskSize* metric includes.


http://gerrit.cloudera.org:8080/#/c/6968/17/src/kudu/tablet/tablet_metadata-test.cc
File src/kudu/tablet/tablet_metadata-test.cc:

PS17, Line 138: The on-disk size 
> nit: maybe "The tablet metadata"?
Done


http://gerrit.cloudera.org:8080/#/c/6968/17/src/kudu/tablet/tablet_replica.cc
File src/kudu/tablet/tablet_replica.cc:

PS17, Line 193: ) != nullptr
> nit: at L701, you have a similar check that doesn't make this explicit comp
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia83f8aae0a544abbd19f5a34520b4828b89b6315
Gerrit-PatchSet: 17
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wdberke...@gmail.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com>
Gerrit-HasComments: Yes

Reply via email to