Mike Percy has posted comments on this change. Change subject: Add some path / env related helper functions ......................................................................
Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/5618/3/src/kudu/util/env_util-test.cc File src/kudu/util/env_util-test.cc: Line 65: Env* env = Env::Default(); > You can use KuduTest's env_ member. Could you fix TestDiskSpaceCheck too? Done PS3, Line 79: We have to clean this up manually > Agreed with Dinesh; test_dir_ will be recursively cleaned up when the test Yeah this is a remnant of a previous revision of this patch where this statement was true. Removed part of this comment. PS3, Line 112: real_dir > Nit: symlink could have been created to test_dir_ itself instead of a new r I like it as a partial part of the path because in your scenario CreateDirsRecursively() would be a no-op. http://gerrit.cloudera.org:8080/#/c/5618/3/src/kudu/util/env_util.cc File src/kudu/util/env_util.cc: Line 29: #include "kudu/gutil/gscoped_ptr.h" > What's this for? apparently not needed, removed PS3, Line 206: Canonicalize > If we move this line to above L200 we don't need to special case symlink he Canonicalize() calls realpath() which can be expensive so this is an optimization. We only call it when it's required. 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 mkdi Done 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 Sp Frankly I don't think SplitPathIntoSegments is a more informative name than SplitPath(). It's just longer. -- 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
