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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[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
