Will Berkeley has posted comments on this change. Change subject: KUDU-1755 Part 2: Improve tablet on-disk size metric ......................................................................
Patch Set 6: (7 comments) http://gerrit.cloudera.org:8080/#/c/6968/6/src/kudu/consensus/consensus_meta.cc File src/kudu/consensus/consensus_meta.cc: Line 220: on_disk_size_.store(pb_.ByteSize(), std::memory_order_relaxed); > yea, I wonder whether it really even is worth tracking this. cmetas are pre I think it's also the only item from 1755 that is essentially fixed size and not proportional to some measure of usage or activity (WAL, UNDOs) or size of data (data, superblock). Mike, would you be ok leaving it out? http://gerrit.cloudera.org:8080/#/c/6968/6/src/kudu/consensus/consensus_meta.h File src/kudu/consensus/consensus_meta.h: Line 182: std::atomic<uint64_t> on_disk_size_; > This class isn't thread-safe so there is no reason to use an atomic here Done http://gerrit.cloudera.org:8080/#/c/6968/6/src/kudu/tablet/diskrowset.h File src/kudu/tablet/diskrowset.h: Line 338: uint64_t OnDiskSize() const OVERRIDE; > Mind adding a comment here to differentiate from OnDiskDataSize()? Done http://gerrit.cloudera.org:8080/#/c/6968/6/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 338: // Return the total size on-disk of this rowset's data, in bytes. > Maybe add (excluding metadata) Done http://gerrit.cloudera.org:8080/#/c/6968/6/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: Line 187: tablet_->GetMetricEntity(), Bind(&TabletReplica::OnDiskSize, Unretained(this))) > In general I don't like metrics that acquire lots of locks on demand since Todd, do you have an idea of how costly gathering these metrics will be? Do you think it'd be worth profiling to see how expensive they are? PS6, Line 360: size_t cmeta_size = consensus_ ? consensus_->MetadataOnDiskSize() : 0; : std::lock_guard<simple_spinlock> lock(lock_); : DCHECK(status_pb_out != nullptr); : status_pb_out->set_tablet_id(meta_->tablet_id()); : status_pb_out->set_table_name(meta_->table_name()); : status_pb_out->set_last_status(last_status_); : meta_->partition().ToPB(status_pb_out->mutable_partition()); : status_pb_out->set_state(state_); : status_pb_out->set_tablet_data_state(meta_->tablet_data_state()); : status_pb_out->set_estimated_on_disk_size(cmeta_size + OnDiskSizeUnlocked()); > Since meta_ is const we can do less under the lock: Done -- To view, visit http://gerrit.cloudera.org:8080/6968 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia83f8aae0a544abbd19f5a34520b4828b89b6315 Gerrit-PatchSet: 6 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: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-HasComments: Yes
