Mike Percy has posted comments on this change.

Change subject: Allow for reserving disk space for non-Kudu processes

Patch Set 4:


Commit Message:

Line 13: reservation limit then a crash will result.
> Why did you choose for the behavior to be a crash instead of just returning
The problem is that if this guy is a leader then he can maintain his leadership 
and never make progress. So it doesn't seem to make sense not to crash.

File src/kudu/integration-tests/disk_reservation-itest.cc:

Line 55: // Note: This functionality is not implemented in the file block 
manager, so
       : // this test is disabled on non-Linux platforms.
> Can't we compile the code on all platforms but, at runtime, test the kind o
What is the benefit of that? I just basically copied what we do for similar 

File src/kudu/util/env.h:

Line 212:   // Returns information about a mounted filesystem.
> No unit test in env-test for StatVfs?
It's literally a pass-through for statvfs().

Line 214:   virtual Status StatVfs(const std::string& path, struct statvfs* 
buf) = 0;
> Not a fan of leaking a POSIX-specific struct into the rest of Kudu. AFAICT 
This was done for expediency; Since this is not an external API it's a decision 
that can be redone later if we need to. However I don't think we have plans to 
support non-POSIX systems anyway so it doesn't seem to be a big problem.

File src/kudu/util/env_posix.cc:

Line 997:       return Status::IOError("statvfs: " + path, 
ErrnoToString(errno), errno);
> You should use the IOError() function so that FLAGS_suicide_on_eio will be 
wasn't aware of that, ok

File src/kudu/util/env_util.cc:

Line 43: Number of inodes to reserve on each filesystem for non-Kudu usage"
> Why would an operator care to reserve the number of inodes? Do other system
This is something Todd suggested we add. I wasn't sure what the default should 
be so I defaulted it to 0.

I can imagine a case where we'd want to avoid taking up all the inodes, 
resulting in a difficult to administer system (potentially preventing login?)

Line 125:   if (PREDICT_FALSE(FLAGS_disk_reserved_bytes_free_for_testing > -1)) 
> If these for_testing flags are set, why bother with the StatVfs() call at a
It keeps the code simple.

To view, visit http://gerrit.cloudera.org:8080/3135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifd0451d4dbddc1783019a53302de0263080939c7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to