Mike Percy has posted comments on this change. Change subject: KUDU-1755 Part 2: Improve tablet on-disk size metric ......................................................................
Patch Set 6: (4 comments) 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()? http://gerrit.cloudera.org:8080/#/c/6968/6/src/kudu/tablet/rowset.h File src/kudu/tablet/rowset.h: Line 338: // Return the total size on-disk of this rowset's data, in bytes. Maybe add (excluding metadata) 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 they impact system load and if you have a monitoring system gathering metrics every few seconds it can hurt. Have you considered whether it's possible to collect this in a lazy or periodic manner? Most of these locks should be fairly fast but it makes me a little nervous 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: DCHECK(status_pb_out != nullptr); uint64_t tablet_size; { std::lock_guard<simple_spinlock> lock(lock_); tablet_size = OnDiskSizeUnlocked(); status_pb_out->set_state(state_); status_pb_out->set_last_status(last_status_); } status_pb_out->set_tablet_id(meta_->tablet_id()); status_pb_out->set_table_name(meta_->table_name()); meta_->partition().ToPB(status_pb_out->mutable_partition()); status_pb_out->set_tablet_data_state(meta_->tablet_data_state()); uint64_t cmeta_size = consensus_ ? consensus_->MetadataOnDiskSize() : 0; status_pb_out->set_estimated_on_disk_size(cmeta_size + tablet_size); -- 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: Will Berkeley <[email protected]> Gerrit-HasComments: Yes
