Mike Percy has posted comments on this change. Change subject: KUDU-1755 Part 2: Improve tablet on-disk size metric ......................................................................
Patch Set 10: (10 comments) Would be nice to add a simple test for the values stored by the new metrics. http://gerrit.cloudera.org:8080/#/c/6968/10/src/kudu/consensus/consensus_meta.h File src/kudu/consensus/consensus_meta.h: Line 153: return on_disk_size_; Can we say this is thread-safe and use an atomic? PS10, Line 241: uint64_t how about atomic and int64_t because there is just no way we are going to need that 64th bit, plus GSG says to prefer signed integers when possible http://gerrit.cloudera.org:8080/#/c/6968/10/src/kudu/consensus/log.cc File src/kudu/consensus/log.cc: PS10, Line 922: CHECK_OK Do we need this CHECK? Also, can we make this safe to run even if log_ is shut down? Some reasonable options: 1. return Status 2. return 0 if not open http://gerrit.cloudera.org:8080/#/c/6968/10/src/kudu/consensus/log.h File src/kudu/consensus/log.h: PS10, Line 201: TotalSize maybe name this OnDiskSize() for consistency? http://gerrit.cloudera.org:8080/#/c/6968/10/src/kudu/consensus/raft_consensus.cc File src/kudu/consensus/raft_consensus.cc: Line 2645: UniqueLock lock(lock_); If we make cmeta_->on_disk_size() thread-safe, then we can avoid taking the consensus lock here. Additional note on why this would be safe: cmeta_ is set only once in Init(), and RaftConsensus::Create() requires Init() to succeed in order to publish the RaftConsensus instance, so we can generally treat the cmeta_ reference as a const. http://gerrit.cloudera.org:8080/#/c/6968/10/src/kudu/tablet/diskrowset.cc File src/kudu/tablet/diskrowset.cc: Line 690: shared_lock<rw_spinlock> l(component_lock_); 1. Can we just take a ref to base_data_ instead of holding the component_lock_ while calling base_data_->OnDiskSize() ? I'm not 100% sure, but it looks like it. 2. Do we need to hold component_lock_ while calling delta_tracker_->OnDiskSize()? I don't see a reason for that, since (a) as long as the DRS is open_, the delta_tracker_ reference is safe to access without a lock because it's set-once and (b) DeltaTracker::OnDiskSize() is thread-safe. http://gerrit.cloudera.org:8080/#/c/6968/10/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: PS10, Line 377: OnDiskSizeNoCMetaUnlocked why the "no cmeta" version? could use a comment PS10, Line 645: auto consensus = consensus_; : if (consensus) { because consensus_ is set-once, this can be written as: if (consensus_) { ret += consensus_->MetadataOnDiskSize(); } PS10, Line 656: size_t Can we use int64_t instead of size_t? We are never going to have 64-bit length data on a single node. See the GSG on unsigned types in general: https://google.github.io/styleguide/cppguide.html#Integer_Types The exceptions to that rule are when storing things like pointers, or when wrapping an existing API makes it basically impossible. PS10, Line 660: if (tablet_) { : ret += tablet_->OnDiskSize(); : } : // WAL segments. : if (state_ == RUNNING) { : ret += log_->TotalSize(); : } Instead of doing this work while holding TabletReplica::lock_, can this be structured so we can just grabs the refs from TabletReplica and then release the lock? Something like: int64_t size = 0; scoped_refptr<Tablet> tablet; scoped_refptr<Log> log; { lock_guard<simple_spinlock> l(lock_); tablet = tablet_; log = log_; } if (tablet) { size += tablet->OnDiskSize(); } if (log) { size += log->OnDiskSize(); } return ret; -- 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: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley <wdberke...@gmail.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-HasComments: Yes
