Dinesh Bhat has posted comments on this change. Change subject: Add some path / env related helper functions ......................................................................
Patch Set 3: Code-Review+1 (5 comments) http://gerrit.cloudera.org:8080/#/c/5618/3/src/kudu/util/env_util-test.cc File src/kudu/util/env_util-test.cc: PS3, Line 79: We have to clean this up manually If this path is part of test directory, do we still have to clean this up manually ? or are we cleaning it up only to verify Deleterecursive routine after CreateRecursive ? PS3, Line 112: real_dir Nit: symlink could have been created to test_dir_ itself instead of a new real_dir. http://gerrit.cloudera.org:8080/#/c/5618/3/src/kudu/util/env_util.cc File src/kudu/util/env_util.cc: PS3, Line 206: Canonicalize If we move this line to above L200 we don't need to special case symlink here. i.e, Canonicalize does more than just resolving symlink, so it's sufficient to check is_dir and continue at L203. http://gerrit.cloudera.org:8080/#/c/5618/3/src/kudu/util/env_util.h File src/kudu/util/env_util.h: PS3, Line 74: . Nit: IMO it would be good to add in the comment that this is emulating mkdir -p. http://gerrit.cloudera.org:8080/#/c/5618/3/src/kudu/util/path_util.cc File src/kudu/util/path_util.cc: PS3, Line 56: SplitPath Do we want to the name of the routine to be bit more intuitive ? Perhaps SplitPathIntoSegments (inline with JoinPathSegments) ? -- 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: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
