[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Will Berkeley (Code Review)]

Will Berkeley has submitted this change and it was merged.

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


KUDU-1755 Part 1: Improve tablet on disk size metric

This adds bloomfile, ad hoc index, and superblock sizes to the on-disk
size metric for tablets. It also renames some on-disk size methods,
clarifying their meaning and removing the word "estimate" because they
are no longer estimates.

A follow up will address log segments and cmeta.

Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Reviewed-on: http://gerrit.cloudera.org:8080/6967
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin 
---
M src/kudu/cfile/bloomfile-test.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction_policy-test.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_info.cc
M src/kudu/tablet/rowset_info.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_history_gc-test.cc
M src/kudu/tablet/tablet_metadata-test.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/util/metrics.h
26 files changed, 153 insertions(+), 66 deletions(-)

Approvals:
  Alexey Serbin: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

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


Patch Set 9: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Will Berkeley (Code Review)]

Hello Mike Percy, Kudu Jenkins,

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

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

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

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

KUDU-1755 Part 1: Improve tablet on disk size metric

This adds bloomfile, ad hoc index, and superblock sizes to the on-disk
size metric for tablets. It also renames some on-disk size methods,
clarifying their meaning and removing the word "estimate" because they
are no longer estimates.

A follow up will address log segments and cmeta.

Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
---
M src/kudu/cfile/bloomfile-test.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction_policy-test.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_info.cc
M src/kudu/tablet/rowset_info.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_history_gc-test.cc
M src/kudu/tablet/tablet_metadata-test.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/util/metrics.h
26 files changed, 153 insertions(+), 66 deletions(-)


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

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

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

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


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6967/7/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:

PS7, Line 284: on_disk_size_.store(superblock.ByteSizeLong(), 
memory_order_relaxed);
> Woops, you are right -- that would not work.
Good catch, thanks! What you suggest works nicely. Done.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

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


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6967/7/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:

PS7, Line 284: on_disk_size_.store(superblock.ByteSizeLong(), 
memory_order_relaxed);
> The Load path is Load -> LoadFromDisk -> LoadFromSuperblock; the Flush path
Woops, you are right -- that would not work.

BTW, ReplaceSuperBlock() is called from TabletCopyClient, and in that case the 
on_disk_size_ would not not updated, if I'm not mistaken.

What do you think of moving that on_disk_size_ update closer to the 'source of 
action' and update it in:

1. LoadFromSuperblock(), covering all read paths
2. ReplaceSuperBlockUnlocked(), covering all write paths

?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

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


Patch Set 5:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/6967/6/src/kudu/tablet/cfile_set.h
File src/kudu/tablet/cfile_set.h:

PS6, Line 75: The size on-disk
> Do you think it's worth clarifying what 'on-disk' means?  Is that the repor
It's the "apparent size" i.e. number of bytes that are serialized from memory 
to disk i.e. the number of bytes that would be sent over the network to 
transfer the data (excluding network metadata). I think this is inline with 
what most people naively think of as the "size on disk" or "file size" for 
something, so I think it's a good name.


http://gerrit.cloudera.org:8080/#/c/6967/6/src/kudu/tablet/compaction_policy-test.cc
File src/kudu/tablet/compaction_policy-test.cc:

PS6, Line 108: int
> hopefully that's not in the order of petabytes in total :)
:)


http://gerrit.cloudera.org:8080/#/c/6967/7/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:

PS7, Line 238: Estimate
> is this still an estimate?
Good catch. Thanks.


http://gerrit.cloudera.org:8080/#/c/6967/7/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:

PS7, Line 284: state_ = kInitialized;
> Maybe, it's worth doing this in LoadFromSuperBlock() instead?  That way it 
The Load path is Load -> LoadFromDisk -> LoadFromSuperblock; the Flush path is 
Flush() -> ReplaceSuperBlockUnlocked(). ReplaceSuperBlockUnlocked doesn't call 
LoadFromSuperblock (although ReplaceSuperBlock, which I think is a test util, 
does), so I don't think your suggestion works.


http://gerrit.cloudera.org:8080/#/c/6967/5/src/kudu/tablet/tablet_metadata.h
File src/kudu/tablet/tablet_metadata.h:

Line 230: return on_disk_size_.load(memory_order_relaxed);
> Yep I'm sure it was macOS being more permissive, I've seen this one before
:(


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Will Berkeley (Code Review)]

Hello Mike Percy, Kudu Jenkins,

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

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

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

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

KUDU-1755 Part 1: Improve tablet on disk size metric

This adds bloomfile, ad hoc index, and superblock sizes to the on-disk
size metric for tablets. It also renames some on-disk size methods,
clarifying their meaning and removing the word "estimate" because they
are no longer estimates.

A follow up will address log segments and cmeta.

Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
---
M src/kudu/cfile/bloomfile-test.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction_policy-test.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_info.cc
M src/kudu/tablet/rowset_info.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_history_gc-test.cc
M src/kudu/tablet/tablet_metadata-test.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/util/metrics.h
26 files changed, 154 insertions(+), 66 deletions(-)


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

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

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

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


Patch Set 7: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6967/5/src/kudu/tablet/tablet_metadata.h
File src/kudu/tablet/tablet_metadata.h:

Line 230: return on_disk_size_.load(std::memory_order_relaxed);
> Yeah :( it compiles on OS X, I guess Jenkins build is stricter about std na
Yep I'm sure it was macOS being more permissive, I've seen this one before


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Alexey Serbin (Code Review)]

Alexey Serbin has posted comments on this change.

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


Patch Set 7:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/6967/6/src/kudu/tablet/cfile_set.h
File src/kudu/tablet/cfile_set.h:

PS6, Line 75: The size on-disk
Do you think it's worth clarifying what 'on-disk' means?  Is that the reported 
size of files or how many disk blocks it takes?

In other words, whether that's more about the output from 'ls -l' or from 'du 
--block-size=1' ('du -b' or 'du --block-size=1')

That's said, maybe the term 'on-disk size' is not the best one for these stats?


http://gerrit.cloudera.org:8080/#/c/6967/6/src/kudu/tablet/compaction_policy-test.cc
File src/kudu/tablet/compaction_policy-test.cc:

PS6, Line 108: int
hopefully that's not in the order of petabytes in total :)


http://gerrit.cloudera.org:8080/#/c/6967/7/src/kudu/tablet/tablet.h
File src/kudu/tablet/tablet.h:

PS7, Line 238: Estimate
is this still an estimate?


http://gerrit.cloudera.org:8080/#/c/6967/7/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:

PS7, Line 284: on_disk_size_.store(superblock.ByteSizeLong(), 
memory_order_relaxed);
Maybe, it's worth doing this in LoadFromSuperBlock() instead?  That way it 
would be automatically updated while calling ReplaceSuperBlockUnlocked() and it 
would not be necessary to call it in Flush()


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Will Berkeley (Code Review)]

Hello Mike Percy, Kudu Jenkins,

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

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

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

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

KUDU-1755 Part 1: Improve tablet on disk size metric

This adds bloomfile, ad hoc index, and superblock sizes to the on-disk
size metric for tablets. It also renames some on-disk size methods,
clarifying their meaning and removing the word "estimate" because they
are no longer estimates.

A follow up will address log segments and cmeta.

Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
---
M src/kudu/cfile/bloomfile-test.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction_policy-test.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_info.cc
M src/kudu/tablet/rowset_info.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_history_gc-test.cc
M src/kudu/tablet/tablet_metadata-test.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/util/metrics.h
26 files changed, 153 insertions(+), 65 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Gerrit-PatchSet: 7
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-1755 Part 1: Improve tablet on disk size metric

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

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


Patch Set 6:

Unrelated failure in spark tests. I'm looking into the failure.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
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 
Gerrit-HasComments: No

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

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


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/6967/5/src/kudu/tablet/rowset.h
File src/kudu/tablet/rowset.h:

Line 115:   virtual Status DebugDump(vector *lines = NULL) = 0;
> warning: default arguments on virtual or override methods are prohibited [g
ignored


http://gerrit.cloudera.org:8080/#/c/6967/4/src/kudu/tablet/rowset_info.cc
File src/kudu/tablet/rowset_info.cc:

PS4, Line 257: rs->MergeCompactionSize()
> In what way are undos not relevant? See MergeUndoHistories() in compaction.
Hm, you're right about that. Based on Todd's comments on previous patches, I 
guess this has worked ok for a while, and changing it may have bad effects it 
will be very difficult to detect or measure, so we shouldn't touch it now.


http://gerrit.cloudera.org:8080/#/c/6967/4/src/kudu/tablet/tablet_metadata.h
File src/kudu/tablet/tablet_metadata.h:

PS4, Line 229: on_disk_size
> Yes I know it's consistent with the style guide, so that's fine. But every 
I'll keep it as is unless you really can't stand it.


http://gerrit.cloudera.org:8080/#/c/6967/5/src/kudu/tablet/tablet_metadata.h
File src/kudu/tablet/tablet_metadata.h:

Line 230: return on_disk_size_.load();
> Looks like you need std::memory_order_relaxed here and in the .cc file
Yeah :( it compiles on OS X, I guess Jenkins build is stricter about std 
namespace members? I've rn into similar whoopsies a couple of times with OS X v 
Jenkins.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Gerrit-PatchSet: 4
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 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Will Berkeley (Code Review)]

Hello Mike Percy, Kudu Jenkins,

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

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

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

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

KUDU-1755 Part 1: Improve tablet on disk size metric

This adds bloomfile, ad hoc index, and superblock sizes to the on-disk
size metric for tablets. It also renames some on-disk size methods,
clarifying their meaning and removing the word "estimate" because they
are not estimates (any more).

A follow up will address log segments and cmeta.

Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
---
M src/kudu/cfile/bloomfile-test.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction_policy-test.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_info.cc
M src/kudu/tablet/rowset_info.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_history_gc-test.cc
M src/kudu/tablet/tablet_metadata-test.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/util/metrics.h
26 files changed, 153 insertions(+), 65 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
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-1755 Part 1: Improve tablet on disk size metric

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

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

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

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

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

KUDU-1755 Part 1: Improve tablet on disk size metric

This adds bloomfile, ad hoc index, and superblock sizes to the on-disk
size metric for tablets. It also renames some on-disk size methods,
clarifying their meaning and removing the word "estimate" because they
are not estimates (any more).

A follow up will address log segments and cmeta.

Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
---
M src/kudu/cfile/bloomfile-test.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction_policy-test.cc
M src/kudu/tablet/delta_tracker.cc
M src/kudu/tablet/delta_tracker.h
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_info.cc
M src/kudu/tablet/rowset_info.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet.h
M src/kudu/tablet/tablet_history_gc-test.cc
M src/kudu/tablet/tablet_metadata-test.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_replica.cc
M src/kudu/tablet/tablet_replica.h
M src/kudu/util/metrics.h
26 files changed, 152 insertions(+), 65 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
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-1755 Part 1: Improve tablet on disk size metric

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

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


Patch Set 4:

(15 comments)

http://gerrit.cloudera.org:8080/#/c/6967/4/src/kudu/tablet/cfile_set.cc
File src/kudu/tablet/cfile_set.cc:

PS4, Line 199: size_t
> uint64_t for consistency
Done


http://gerrit.cloudera.org:8080/#/c/6967/4/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:

PS4, Line 885: OnDiskSize
> MergeCompactionSize?
The message says current size on disk so OnDiskSize is the most accurate. I 
think including the compaction-relevant measure would be confusing since it 
won't match with metrics.


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

PS4, Line 677: CFile
> Maybe BaseDataOnDiskSize()... I'm not sure CFile is the right term here bec
BaseDataOnDiskSize; it's only used in tests but it ends up being used in patch 
2 :)


PS4, Line 686: OnDiskDataSize
> Maybe BaseDataOnDiskSizeExcludingMetadata() ? It's a bit wordy though.
BaseDataOnDiskSizeNoMetadata()


PS4, Line 701: MergeCompactionSize
> This is used for Major Delta Compaction, I don't think it's used for Merge 
It's part of the info in RowsetInfo and in turn in a CompactionPolicy, so I 
think it's used for merging rowsets too. That might've been the original idea 
behind calling it Compaction size since it represents the measure of the rowset 
for multiple compaction types.


http://gerrit.cloudera.org:8080/#/c/6967/4/src/kudu/tablet/rowset.h
File src/kudu/tablet/rowset.h:

Line 115:   virtual Status DebugDump(vector *lines = NULL) = 0;
> warning: default arguments on virtual or override methods are prohibited [g
ignored


Line 121:   virtual uint64_t MergeCompactionSize() const = 0;
> Maybe DataSizeWithoutUndoDeltas() or something
OnDiskDataSizeNoUndos()


http://gerrit.cloudera.org:8080/#/c/6967/4/src/kudu/tablet/rowset_info.cc
File src/kudu/tablet/rowset_info.cc:

PS4, Line 257: rs->MergeCompactionSize()
> Do you know why we wouldn't want undo deltas included in this estimation?
Undos aren't relevant for merging rowsets. Since compaction of rowsets also 
applies their redos we want to look at the base data size and the redos.


http://gerrit.cloudera.org:8080/#/c/6967/4/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:

Line 1470:  input.rowsets()[0]->MergeCompactionSize() == 0) {
> This seems to just be a proxy for detecting that we flushed the MRS
Is that not ok? Seems like it works here and I don't see another existing 
method to do the same thing.


http://gerrit.cloudera.org:8080/#/c/6967/4/src/kudu/tablet/tablet_history_gc-test.cc
File src/kudu/tablet/tablet_history_gc-test.cc:

Line 240
> I would like to keep equivalent assertions here if possible, perhaps using 
Done


Line 313
> Same
Done


http://gerrit.cloudera.org:8080/#/c/6967/4/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:

Line 283:   on_disk_size_.store(superblock.ByteSizeLong());
> memory_order_relaxed
Done


Line 477:   on_disk_size_.store(pb.ByteSizeLong());
> memory_order_relaxed
Done


http://gerrit.cloudera.org:8080/#/c/6967/4/src/kudu/tablet/tablet_metadata.h
File src/kudu/tablet/tablet_metadata.h:

PS4, Line 229: on_disk_size
> Should we name this OnDiskSize() for consistency with the rest of the metho
GSG says accessors *may* be named like this instead of like "OnDiskSize", but 
it's near-universal in the Kudu code base to name like this, AFAICT...your call.


Line 230: return on_disk_size_.load();
> memory_order_relaxed
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Gerrit-PatchSet: 4
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 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

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


Patch Set 4:

(14 comments)

http://gerrit.cloudera.org:8080/#/c/6967/4/src/kudu/tablet/cfile_set.cc
File src/kudu/tablet/cfile_set.cc:

PS4, Line 199: size_t
uint64_t for consistency


http://gerrit.cloudera.org:8080/#/c/6967/4/src/kudu/tablet/compaction.cc
File src/kudu/tablet/compaction.cc:

PS4, Line 885: OnDiskSize
MergeCompactionSize?


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

PS4, Line 677: CFile
Maybe BaseDataOnDiskSize()... I'm not sure CFile is the right term here because 
IIRC delta files are actually implemented as cfiles. However, is this even used?


PS4, Line 686: OnDiskDataSize
Maybe BaseDataOnDiskSizeExcludingMetadata() ? It's a bit wordy though.


PS4, Line 701: MergeCompactionSize
This is used for Major Delta Compaction, I don't think it's used for Merge 
Compaction. Sorry if I wasn't clear earlier


http://gerrit.cloudera.org:8080/#/c/6967/4/src/kudu/tablet/rowset.h
File src/kudu/tablet/rowset.h:

Line 121:   virtual uint64_t MergeCompactionSize() const = 0;
Maybe DataSizeWithoutUndoDeltas() or something


http://gerrit.cloudera.org:8080/#/c/6967/4/src/kudu/tablet/rowset_info.cc
File src/kudu/tablet/rowset_info.cc:

PS4, Line 257: rs->MergeCompactionSize()
Do you know why we wouldn't want undo deltas included in this estimation?


http://gerrit.cloudera.org:8080/#/c/6967/4/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:

Line 1470:  input.rowsets()[0]->MergeCompactionSize() == 0) {
This seems to just be a proxy for detecting that we flushed the MRS


http://gerrit.cloudera.org:8080/#/c/6967/4/src/kudu/tablet/tablet_history_gc-test.cc
File src/kudu/tablet/tablet_history_gc-test.cc:

Line 240
I would like to keep equivalent assertions here if possible, perhaps using 
something like tablet()->OnDiskDataSize() ... either that or we can have Tablet 
friend this class and call into private methods to verify that the data was 
deleted.


Line 313
Same


http://gerrit.cloudera.org:8080/#/c/6967/4/src/kudu/tablet/tablet_metadata.cc
File src/kudu/tablet/tablet_metadata.cc:

Line 283:   on_disk_size_.store(superblock.ByteSizeLong());
memory_order_relaxed


Line 477:   on_disk_size_.store(pb.ByteSizeLong());
memory_order_relaxed


http://gerrit.cloudera.org:8080/#/c/6967/4/src/kudu/tablet/tablet_metadata.h
File src/kudu/tablet/tablet_metadata.h:

PS4, Line 229: on_disk_size
Should we name this OnDiskSize() for consistency with the rest of the methods?


Line 230: return on_disk_size_.load();
memory_order_relaxed


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Gerrit-PatchSet: 4
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 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

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


Patch Set 3:

Actually I'm switching to "Tablet total size on disk" and "Tablet data size on 
disk"

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Gerrit-PatchSet: 3
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 
Gerrit-HasComments: No

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

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

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

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

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

KUDU-1755 Part 1: Improve tablet on disk size metric

This adds bloomfile, ad hoc index, and superblock sizes to the on-disk
size metric for tablets. It also renames some on-disk size methods,
clarifying their meaning and removing the word "estimate" because they
are not estimates (any more).

A follow up will address log segments and cmeta.

Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
---
M src/kudu/cfile/bloomfile-test.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/compaction.cc
M src/kudu/tablet/compaction_policy-test.cc
M src/kudu/tablet/diskrowset-test.cc
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/memrowset.h
M src/kudu/tablet/mock-rowsets.h
M src/kudu/tablet/rowset.cc
M src/kudu/tablet/rowset.h
M src/kudu/tablet/rowset_info.cc
M src/kudu/tablet/rowset_info.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_history_gc-test.cc
M src/kudu/tablet/tablet_metadata-test.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_replica.h
M src/kudu/util/metrics.h
22 files changed, 129 insertions(+), 50 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

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


Patch Set 2:

> Also, I wonder if we shouldn't introduce an additional metric which
 > would capture these additional types of disk usage. This is getting
 > into metadata territory and I think having separate metrics
 > measuring data stored and total size on disk including metadata
 > would be useful.

That's a good idea. It would make sense to have a "Tablet size on-disk" and a 
"Tablet metadata size on-disk", I think. I'll add the later into part 2 (as 
that change logically fits there).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

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


Patch Set 2:

(4 comments)

I changed the names to
- Remove the word "Estimate" because they aren't estimates.
- Get rid of references to "compaction" by naming the measure by what it is, 
not what it is used for. The remaining *Compaction* method is 
MergeCompactionSize, which isn't anything sensible except for its role in 
compactions, so I named it after the specific kind of compaction it's used for.

http://gerrit.cloudera.org:8080/#/c/6967/2/src/kudu/tablet/cfile_set.cc
File src/kudu/tablet/cfile_set.cc:

Line 199:   uint64_t ret = 0;
> This can call into the function below for this first loop
Done


PS2, Line 212: :EstimateCompactionSize
> How about EstimateOnDiskDataSize? And then doc it as actual data stored on 
Done


http://gerrit.cloudera.org:8080/#/c/6967/2/src/kudu/tablet/cfile_set.h
File src/kudu/tablet/cfile_set.h:

Line 71:   uint64_t EstimateOnDiskSize() const;
> We should doc this too
Done


Line 73:   uint64_t EstimateCompactionSize() const;
> Needs doc
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

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


Patch Set 2:

Also, I wonder if we shouldn't introduce an additional metric which would 
capture these additional types of disk usage. This is getting into metadata 
territory and I think having separate metrics measuring data stored and total 
size on disk including metadata would be useful.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Mike Percy (Code Review)]

Mike Percy has posted comments on this change.

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


Patch Set 2:

(4 comments)

I sort of have a problem with referring to it as "compaction size" because 
there are a bunch of different types of compactions (see 
https://github.com/apache/kudu/blob/master/docs/design-docs/tablet.md#delta-compactions
 )

Can we come up with a more descriptive name for this stuff?

http://gerrit.cloudera.org:8080/#/c/6967/2/src/kudu/tablet/cfile_set.cc
File src/kudu/tablet/cfile_set.cc:

Line 199:   uint64_t ret = 0;
This can call into the function below for this first loop


PS2, Line 212: :EstimateCompactionSize
How about EstimateOnDiskDataSize? And then doc it as actual data stored on disk


http://gerrit.cloudera.org:8080/#/c/6967/2/src/kudu/tablet/cfile_set.h
File src/kudu/tablet/cfile_set.h:

Line 71:   uint64_t EstimateOnDiskSize() const;
We should doc this too


Line 73:   uint64_t EstimateCompactionSize() const;
Needs doc


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy 
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Will Berkeley (Code Review)]

Hello Kudu Jenkins,

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

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

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

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

KUDU-1755 Part 1: Improve tablet on disk size metric

This adds bloomfile, ad hoc index, and superblock sizes to the on-disk
size metric for tablets.

A follow up will address log segments and cmeta.

Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
---
M src/kudu/cfile/bloomfile-test.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/diskrowset.h
M src/kudu/tablet/rowset_info.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_history_gc-test.cc
M src/kudu/tablet/tablet_metadata-test.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_replica.h
M src/kudu/util/metrics.h
14 files changed, 87 insertions(+), 10 deletions(-)


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

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

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Will Berkeley (Code Review)]

Will Berkeley has posted comments on this change.

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


Patch Set 1:

I just want a Jenkins run.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley 
Gerrit-HasComments: No

[kudu-CR] KUDU-1755 Part 1: Improve tablet on disk size metric

[Will Berkeley (Code Review)]

Will Berkeley has uploaded a new change for review.

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

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

KUDU-1755 Part 1: Improve tablet on disk size metric

This adds bloomfile, ad hoc index, and superblock sizes to the on-disk
size metric for tablets.

A follow up will address log segments and cmeta.

Change-Id: I32dce598bbb8e18325210a49fc436fd0f7ac68fd
---
M src/kudu/cfile/bloomfile-test.cc
M src/kudu/cfile/bloomfile.h
M src/kudu/tablet/cfile_set.cc
M src/kudu/tablet/cfile_set.h
M src/kudu/tablet/diskrowset.cc
M src/kudu/tablet/rowset_info.h
M src/kudu/tablet/tablet.cc
M src/kudu/tablet/tablet_metadata-test.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
M src/kudu/tablet/tablet_replica.h
M src/kudu/util/metrics.h
12 files changed, 76 insertions(+), 5 deletions(-)


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

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