Todd Lipcon 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.
> We talked about this on Slack. I commented on this before looking at the ac
agreed, I think the log should just return IOError, and if the leader gets an
IOError on a log write, it should crash. (I thought it already did, in fact)
Line 1058: ASSERT_TRUE(s.IsServiceUnavailable());
I'm not sold on ServiceUnavailable as the status here - typically that's used
for a _temporary_ condition, and some higher level stuff will retry due to it.
Line 847: Assert
the word Assert here sounds like a testing thing or a CHECK. Maybe
VerifySufficientDiskSpace? or CheckSufficientDiskSpace? (for some reason we use
Check not just in the context of CHECK)
Line 62: DEFINE_int32(full_disk_cache_seconds, 30,
probably should have a stability flag. maybe advanced/unstable for the first
Line 860: RETURN_NOT_OK_PREPEND(AppendMetadata(), "Unable to flush block");
shouldn't this say Unable to append block metadata?
Line 1222: unordered_set<int> full_root_indexes(root_paths_.size());
why not initialize this from the full disk cache?
Line 1250: if (full_disk_cache_.IsRootFull(root_path,
sort of odd to have to pass in the current time here
Line 1285: full_disk_cache_.MarkRootFull(container->root_path(),
again seems like the expiry calculation might make more sense inside the cache
Line 1349: "Unable to delete block");
append deletion record?
Line 1402: metrics()->unavailable_containers->IncrementBy(-1);
I think better to queue up all the metrics adjustment until the end, and then
just apply it once. Otherwise someone fetching metrics could easily hit a race
where the number is inaccurate
Line 156: bool IsRootFull(const std::string& root_path, MonoTime now,
MonoTime* expires = nullptr) const;
Line 17: #include <sys/statvfs.h>
any idea if this is OSX-compatible or does this functionality need some ifdef
Line 43: Number of inodes to reserve on each filesystem for non-Kudu usage"
> I'd like to see a stronger justification before this is added. Yes, we can
I was actually thinking about this more in terms of the file block manager, but
since it sounds like we aren't doing FBM support anyway (since the code landed
at the LBM layer) I'd agree it's not super important. With FBM, we do chew up a
_lot_ of inodes, and it's not uncommon to see them running out (have seen this
hit HDFS datanodes before)
Line 120: RETURN_NOT_OK(env->StatVfs(path, &buf));
do we have any idea the perf impact of a StatVfs syscall? My guess is that it's
always fast and so this won't be noticeable, but I was half wondering whether
the "full disk cache" thing might be better done as a "statvfs cache"
Line 45: Status AssertSufficientDiskSpace(Env *env, const std::string& path,
> Nit: maybe replace "Assert" with "Check" or "Verify"? When I see assert I t
oh hey, I said the same thing elsewhere
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>