Mike Percy has posted comments on this change. Change subject: Allow for reserving disk space for non-Kudu processes ......................................................................
Patch Set 4: (7 comments) http://gerrit.cloudera.org:8080/#/c/3135/4//COMMIT_MSG 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. http://gerrit.cloudera.org:8080/#/c/3135/4/src/kudu/integration-tests/disk_reservation-itest.cc 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 tests. http://gerrit.cloudera.org:8080/#/c/3135/4/src/kudu/util/env.h 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. http://gerrit.cloudera.org:8080/#/c/3135/4/src/kudu/util/env_posix.cc 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 http://gerrit.cloudera.org:8080/#/c/3135/4/src/kudu/util/env_util.cc 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
