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 <[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
