[kudu-CR] Allow for reserving disk space for non-Kudu processes

["Mike Percy (Code Review)" <ge...@cloudera.org>]

Mike Percy has posted comments on this change.

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


Patch Set 4:

(24 comments)

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

Line 13: reservation limit then a crash will result.
> agreed, I think the log should just return IOError, and if the leader gets 
There is a series now before this patch that fixes the existing bugs I found in this implementation and cleans up the interface.


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 us
Using IOError w/ ENOSPC posix code.


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

PS4, Line 847: Assert
> the word Assert here sounds like a testing thing or a CHECK. Maybe VerifySu
Done


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


Line 860:     RETURN_NOT_OK_PREPEND(AppendMetadata(), "Unable to flush block");
> shouldn't this say Unable to append block metadata?
Done


Line 1222:   unordered_set<int> full_root_indexes(root_paths_.size());
> why not initialize this from the full disk cache?
Done


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
Done


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


Line 1349:                         "Unable to delete block");
> append deletion record?
Done


Line 1402:       metrics()->unavailable_containers->IncrementBy(-1);
> I think better to queue up all the metrics adjustment until the end, and th
Done


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
Done


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:

PS4, Line 55: // Note: This functionality is not implemented in the file block manager, so
            : // this test is disabled on non-Linux platforms.
> It's more flexible: even as Kudu platform support evolves and changes, we w
I changed it to rely on the default value of the --block_manager flag


PS4, Line 67:   workload.set_num_write_threads(4);
            :   workload.set_timeout_allowed(true);
            :   workload.set_write_timeout_millis(500);
> How did you arrive at these non-default values? If they're important, they'
# threads isn't actually needed. Added a comment for the others.


PS4, Line 73:   // Write a few rows to get some rows flushing.
            :   while (workload.rows_inserted() < 10) {
            :     SleepFor(MonoDelta::FromMilliseconds(10));
            :   }
> Why do we need to do this if we're also looping and waiting for containers 
Removed


Line 80:   // Wait until we have 2 active containers on TS1.
> Nit: TS1 is the only TS; why refer to it as TS1? Below too.
No good reason. Removed the name.


Line 95:                                          1L * 1024 * 1024 * 1024)));
> 1GB seems unnecessarily high. How about 128 MB, or even lower? How long doe
We're not waiting for that to fill up. Added comments to clarify.


Line 126:   ts_flags.push_back("--enable_maintenance_manager=false"); // No flushing for this test.
> Why is this important?
It used to be important, since we shared flags between the data dirs and the log dirs. I've since removed it.


PS4, Line 132:   workload.set_timeout_allowed(true);
             :   workload.set_write_timeout_millis(500);
             :   workload.set_num_write_threads(8);
             :   workload.set_write_batch_size(1024);
> As above, please justify the non-default values with comments.
Done


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;
> Your counterargument is not wrong, but it's one we could use when adding an
I reduced it down to a GetFreeBytes() call


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 ifd
looks like it's POSIX and supported by MacOS: https://developer.apple.com/library/ios/documentation/System/Conceptual/ManPages_iPhoneOS/man3/fstatvfs.3.html


Line 993:   virtual Status StatVfs(const std::string& path, struct statvfs* buf) OVERRIDE {
> Need to add ThreadRestrictions::AssertIOAllowed(). Also should add a TRACE_
Done


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

PS4, Line 43: Number of inodes to reserve on each filesystem for non-Kudu usage"
> I was actually thinking about this more in terms of the file block manager,
We can always add it back later if we want it. Removing the inode check.


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 
I've abstracted out StatVfs() as an Env::GetFreeBytes() call. Let me know if you want me to revisit this.


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);
> oh hey, I said the same thing elsewhere
Done


-- 
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes