Adar Dembo has posted comments on this change.

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


Patch Set 8:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/3135/8/src/kudu/fs/block_manager-test.cc
File src/kudu/fs/block_manager-test.cc:

PS8, Line 968: // Let the disk-full cache expire.
             :   SleepFor(MonoDelta::FromMilliseconds(100));
Do we still need this sleep now that we're using a FINE clock?


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

Line 1292:     }
> Yes, preallocation sounds a little bit broken.
I see your point (regarding soft vs. hard limits). So let's proceed with what 
you have, provided:
1. Add a note to the help for data_dirs_reserved_bytes explaining that it'll 
only work when log_container_preallocate_bytes is non-zero.
2. Add a comment here explaining the "preallocation size as number of bytes we 
think we're about to consume" inaccuracy.


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

PS8, Line 1234: Initialize the
              :   // indexes from the FullDiskCache.
Nit: it's not using indexes anymore.


Line 1240:       full_root_paths.insert(root_paths_[i]);
Why not InsertOrDie() here and below? I don't think root_paths_ can have 
duplicates, can it?


PS8, Line 1302: "Log block manager: Insufficient disk space under path "
              :                    << container->root_path() << ": Creation of 
new data blocks under this path "
              :                    << "can be retried after " << 
FLAGS_log_block_manager_full_disk_cache_seconds
              :                    << " seconds: " << s.ToString();
Nit: I think getting the gist of the entire line would be easier if you used 
Substitute() to build it.


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

Line 46: TAG_FLAG(disk_reserved_prefixes_with_bytes_free_for_testing, unsafe);
> I'm changing them in the integration tests.
Ah, didn't notice that.


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