Adar Dembo has posted comments on this change.

Change subject: KUDU-1755 Part 2: Improve tablet on-disk size metric
......................................................................


Patch Set 9:

(12 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.


Line 322:   WARN_NOT_OK(cmeta->UpdateOnDiskSize(), "Failed to update cmeta 
on-disk size");
And here.


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_;
Why doesn't this need to be atomic (like TabletReplica::on_disk_size_)?


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 these 
functions describing the different space-occupying parts of a DRS?


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 OnDiskSize() 
should include _all_ metadata. That is, not just the superblock but also the 
cmeta. But of course we can't do that since the cmeta isn't known here.

Can you update the comments (here and to OnDiskDataSize) to clarify this?


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.


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'll 
return failure even though we did replace the superblock". The counter-argument 
is: "WritePBContainerToPath can fail and still replace the superblock (i.e. a 
failure in SyncDir)".


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?


Line 377:     tablet_size = OnDiskSizeUnlocked();
We add in cmeta_size below and this is the only use of OnDiskSizeUnlocked. Why 
not make OnDiskSizeUnlocked return the cmeta size too?


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 it 
for nullitude. Or you need to make the call with lock_ held, which guarantees 
that consensus_ isn't modified.

Okay, I went and read the comments in TabletReplica which talk about how 
consensus_ is a "set-once" object. That seems bogus to me, though admittedly 
I'm not an expert on this code. TabletReplica::Shutdown() resets consensus_ 
with lock_ held. Can we guarantee that when Shutdown() is called, no other 
thread (besides the one calling Shutdown()) will invoke a TabletReplica 
function? I don't see how we can (hence the fragility). And if we can't, then 
consensus_ access needs to be atomic, either by taking a local ref then 
operating on it, or by acquiring lock_ first.


Line 640: size_t TabletReplica::OnDiskSize() const {
Normally the only distinction between Foo and FooUnlocked is the acquisition of 
a lock. Here OnDiskSizeUnlocked does not include the consensus metadata size.

Can you rename the functions accordingly?


Line 656: size_t TabletReplica::OnDiskSizeUnlocked() const {
Can you DCHECK that lock_ is held?


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

Reply via email to