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/05/23 23:16:20 UTC

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

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 <wd...@gmail.com>

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

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

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

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <as...@cloudera.com>
---
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 <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] KUDU-1755 Part 1: Improve tablet on disk size metric

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <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] KUDU-1755 Part 1: Improve tablet on disk size metric

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <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] KUDU-1755 Part 1: Improve tablet on disk size metric

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

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

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <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] KUDU-1755 Part 1: Improve tablet on disk size metric

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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<string> *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 <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] KUDU-1755 Part 1: Improve tablet on disk size metric

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/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 <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

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

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/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 <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] KUDU-1755 Part 1: Improve tablet on disk size metric

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

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

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <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] KUDU-1755 Part 1: Improve tablet on disk size metric

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <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] KUDU-1755 Part 1: Improve tablet on disk size metric

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <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] KUDU-1755 Part 1: Improve tablet on disk size metric

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

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

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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<string> *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 <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] KUDU-1755 Part 1: Improve tablet on disk size metric

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

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


Patch Set 5: Code-Review+1

(4 comments)

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

PS4, Line 885: OnDiskSize
> The message says current size on disk so OnDiskSize is the most accurate. I
hmm, ok that's fair


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->OnDiskDataSizeNoUndos
> Undos aren't relevant for merging rowsets. Since compaction of rowsets also
In what way are undos not relevant? See MergeUndoHistories() in compaction.cc

That said, I wouldn't advocate for changing that in this patch, I'm just not sure of the reasoning for not doing including them.


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
> GSG says accessors *may* be named like this instead of like "OnDiskSize", b
Yes I know it's consistent with the style guide, so that's fine. But every other method in this patch has the other naming style for the same thing so it seems incongruous to me.


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);
> error: use of undeclared identifier 'memory_order_relaxed'; did you mean 's
Looks like you need std::memory_order_relaxed here and in the .cc file


-- 
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 <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] KUDU-1755 Part 1: Improve tablet on disk size metric

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

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

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <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] KUDU-1755 Part 1: Improve tablet on disk size metric

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <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] KUDU-1755 Part 1: Improve tablet on disk size metric

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <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] KUDU-1755 Part 1: Improve tablet on disk size metric

Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <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] KUDU-1755 Part 1: Improve tablet on disk size metric

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <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] KUDU-1755 Part 1: Improve tablet on disk size metric

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
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 <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] KUDU-1755 Part 1: Improve tablet on disk size metric

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/6967

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

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_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
24 files changed, 140 insertions(+), 61 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/67/6967/4
-- 
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: 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: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

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

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/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 <wd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

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

Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
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 <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