Will Berkeley has posted comments on this change. Change subject: KUDU-1755 Part 2: Improve tablet on-disk size metric ......................................................................
Patch Set 9: (13 comments) http://gerrit.cloudera.org:8080/#/c/6968/9/src/kudu/consensus/consensus_meta.cc File src/kudu/consensus/consensus_meta.cc: Line 285: WARN_NOT_OK(UpdateOnDiskSize(), "Failed to update cmeta on-disk size"); > Could RETURN_NOT_OK here too. Done Line 322: WARN_NOT_OK(cmeta->UpdateOnDiskSize(), "Failed to update cmeta on-disk size"); > And here. Done http://gerrit.cloudera.org:8080/#/c/6968/9/src/kudu/consensus/consensus_meta.h File src/kudu/consensus/consensus_meta.h: Line 241: uint64_t on_disk_size_; > Ah, I just read your earlier discussion with Mike. Nevermind. Right, class isn't threadsafe. http://gerrit.cloudera.org:8080/#/c/6968/9/src/kudu/tablet/diskrowset.h File src/kudu/tablet/diskrowset.h: PS9, Line 331: // Return the on-disk size of this rowset's cfile set, including bloomfiles : // and the ad hoc index, in bytes. : uint64_t BaseDataOnDiskSize() const; : : // Return the size on-disk of this rowset's REDO deltas, in bytes. : uint64_t RedoDeltaOnDiskSize() const; : : // Return the size on-disk of this rowset, in bytes. : uint64_t OnDiskSize() const OVERRIDE; : : // Return the size on-disk of the data in this rowset (i.e. excluding metadata), in bytes. : uint64_t OnDiskDataSize() const OVERRIDE; > The number of parts is high; could you add a block comment just before thes Done http://gerrit.cloudera.org:8080/#/c/6968/9/src/kudu/tablet/rowset.h File src/kudu/tablet/rowset.h: Line 115: virtual Status DebugDump(std::vector<std::string> *lines = NULL) = 0; > warning: default arguments on virtual or override methods are prohibited [g Ignored. http://gerrit.cloudera.org:8080/#/c/6968/9/src/kudu/tablet/tablet.h File src/kudu/tablet/tablet.h: Line 237: // Return the total on-disk size of this tablet, in bytes. > Without additional commenting, it seems like the value returned by OnDiskSi Done http://gerrit.cloudera.org:8080/#/c/6968/9/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: Line 309: WARN_NOT_OK(UpdateOnDiskSize(), "Failed to update superblock on-disk size"); > Seems reasonable to RETURN_NOT_OK here. Done Line 587: WARN_NOT_OK(UpdateOnDiskSize(), "Failed to update superblock on-disk size"); > Maybe even RETURN_NOT_OK here too. I think your fear is "if this fails, we' Done http://gerrit.cloudera.org:8080/#/c/6968/9/src/kudu/tablet/tablet_replica.cc File src/kudu/tablet/tablet_replica.cc: Line 87: "Space used by this tablet on disk."); > Maybe mention that this includes both data and metadata? Done Line 377: tablet_size = OnDiskSizeUnlocked(); > We add in cmeta_size below and this is the only use of OnDiskSizeUnlocked. Because it causes a potential deadlock-- getting the cmeta size requires taking a lock in consensus, so OnDiskSize would acquire the locks in the order tablet_replica lock -> consensus lock, but (IIRC) the transaction code acquires the locks in the order consensus lock -> tablet_replica lock. There was a TSAN failure showing this in the pre-commit jobs but it looks like the links are broken now :/ Line 385: size_t cmeta_size = consensus_ ? consensus_->MetadataOnDiskSize() : 0; > This isn't safe; you need to take a local ref of consensus_ and then test i I'll take a local ref. Line 640: size_t TabletReplica::OnDiskSize() const { > Normally the only distinction between Foo and FooUnlocked is the acquisitio Done Line 656: size_t TabletReplica::OnDiskSizeUnlocked() const { > Can you DCHECK that lock_ is held? 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: 9 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
