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 <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Alexey Serbin <[email protected]>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <[email protected]>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Will Berkeley <[email protected]>
Gerrit-HasComments: Yes

Reply via email to