Adar Dembo has posted comments on this change.
Change subject: Allow for reserving disk space for non-Kudu processes
Patch Set 4:
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.
Line 55: // Note: This functionality is not implemented in the file block
: // 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
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.
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-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>