Todd Lipcon has posted comments on this change.

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


Patch Set 4:

(15 comments)

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

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)


http://gerrit.cloudera.org:8080/#/c/3135/4/src/kudu/consensus/log-test.cc
File src/kudu/consensus/log-test.cc:

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.


http://gerrit.cloudera.org:8080/#/c/3135/4/src/kudu/consensus/log.cc
File src/kudu/consensus/log.cc:

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)


http://gerrit.cloudera.org:8080/#/c/3135/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:

Line 62: DEFINE_int32(full_disk_cache_seconds, 30,
probably should have a stability flag. maybe advanced/unstable for the first 
version?


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, 
MonoTime::Now(MonoTime::COARSE))) {
sort of odd to have to pass in the current time here


Line 1285:         full_disk_cache_.MarkRootFull(container->root_path(), 
expires);
again seems like the expiry calculation might make more sense inside the cache 
method iself


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


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

Line 156:   bool IsRootFull(const std::string& root_path, MonoTime now, 
MonoTime* expires = nullptr) const;
docs


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

Line 17: #include <sys/statvfs.h>
any idea if this is OSX-compatible or does this functionality need some ifdef 
__apple__s?


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


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

Line 45: Status AssertSufficientDiskSpace(Env *env, const std::string& path, 
int64_t bytes);
> 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-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