Todd Lipcon has posted comments on this change. Change subject: env_util: Implement CreateDirsRecursively() ......................................................................
Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/5618/1/src/kudu/util/env_util-test.cc File src/kudu/util/env_util-test.cc: Line 72: // Relative path. We should clean this up manually. hrm, do we have any guarantee about what the cwd is in the test context? I have no idea, but maybe we should chdir somewhere first? Line 81: } how about some test cases for failure cases? mkdir on top of an existing file? mkdir on top of an existing directory? mkdir on top of an existing prefix? http://gerrit.cloudera.org:8080/#/c/5618/1/src/kudu/util/env_util.cc File src/kudu/util/env_util.cc: Line 196: if (env->FileExists(path)) break; this seems like it will return Status::OK even if 'path' exists and is a file. Maybe this should instead be checking if it's a directory? If it's a file return AlreadyExists or something? Line 202: for (auto path = paths.crbegin(); path != paths.crend(); ++path) { I find the logic of this function somewhat hard to follow as written (accumulating a list of directories to create and then creating them in reverse order). Couldn't it be written more simply as something like: const auto& components = Split(path, '/'); string prefix; for (const auto& c : components) { prefix = prefix.empty() ? c : JoinPathSegments(prefix, c); if (!env->IsDirectory(prefix)) { RETURN_NOT_OK(env->CreateDir(prefix)); } } ... or am I missing something? -- To view, visit http://gerrit.cloudera.org:8080/5618 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia664708a09493923abbf2ff4e56e3d49c62cf97e Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
