Adar Dembo has posted comments on this change.

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


Patch Set 4:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/3135/4//COMMIT_MSG
Commit Message:

Line 13: reservation limit then a crash will result.
> The problem is that if this guy is a leader then he can maintain his leader
We talked about this on Slack. I commented on this before looking at the actual 
code, and in this patch whenever you check for reserved space you do return 
errors rather than inducing crashes. Perhaps you can reword the description a 
bit so it's more clear that you're referring to the system as a whole rather 
than your new code?

As for your point about leadership and not making progress, I agree, but that's 
more relevant to the previous patch (where you've added LOG(FATAL) in the event 
of an IO failure), so we should continue the discussion there.


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.
> What is the benefit of that? I just basically copied what we do for similar
It's more flexible: even as Kudu platform support evolves and changes, we won't 
ever need to revisit this.

If doing it is a huge pain then I'll relent, but I don't think it is. As for 
the other tests, if I was around to review the Mac OS changes when they landed 
(I think it was while I was away on honeymoon) I would have said the exact same 
thing. :)


http://gerrit.cloudera.org:8080/#/c/3135/4/src/kudu/util/env.h
File src/kudu/util/env.h:

Line 214:   virtual Status StatVfs(const std::string& path, struct statvfs* 
buf) = 0;
> This was done for expediency; Since this is not an external API it's a deci
Your counterargument is not wrong, but it's one we could use when adding 
anything new to Env. Note that we've always taken care to be platform-agnostic 
here (e.g. Walk() doesn't expose fts* details) and I'd rather we didn't change 
that precedent now, since it's hard to reel back in once undone.

If it helps, I've found that these sort of precedents pay dividends down the 
line, and typically don't result in heavy "taxes" on a patch by patch basis.


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"
> This is something Todd suggested we add. I wasn't sure what the default sho
I'd like to see a stronger justification before this is added. Yes, we can add 
a control for it, but why is it useful? As a system Kudu doesn't really hog 
inodes since it tends to cram large amounts of data into each file, so I don't 
see the use case.

Do you remember what Todd's argument was? Maybe he can chime in.


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