You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2017/01/05 21:15:25 UTC
[kudu-CR] env util: Implement CreateDirsRecursively()
Hello Dinesh Bhat, Lars Volker, Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5618
to review the following change.
Change subject: env_util: Implement CreateDirsRecursively()
......................................................................
env_util: Implement CreateDirsRecursively()
This function is similar in spirit to the "mkdir -p" command.
Also reordered a few includes and deleted a few lines of commented-out code.
Change-Id: Ia664708a09493923abbf2ff4e56e3d49c62cf97e
---
M src/kudu/util/env_util-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
3 files changed, 62 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/5618/1
--
To view, visit http://gerrit.cloudera.org:8080/5618
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia664708a09493923abbf2ff4e56e3d49c62cf97e
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
[kudu-CR] env util: Implement CreateDirsRecursively()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: env_util: Implement CreateDirsRecursively()
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5618/2/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:
Line 200: if (s.ok() && is_dir) continue;
If you're aiming for mkdir -p semantics, it should be ok for one of the path components to be a valid symlink to a directory. I don't think this implementation satisfies that.
Here's an example of what mkdir will do:
adar@adar-ThinkPad-T540p:/tmp$ ls -dl a b
drwxrwxr-x 3 adar adar 4096 Jan 5 17:35 a
lrwxrwxrwx 1 adar adar 1 Jan 5 17:34 b -> a
adar@adar-ThinkPad-T540p:/tmp$ strace mkdir -p b/c/d
...
mkdir("b", 0777) = -1 EEXIST (File exists)
chdir("b") = 0
mkdir("c", 0777) = 0
open("c", O_RDONLY|O_NOCTTY|O_NONBLOCK|O_DIRECTORY|O_NOFOLLOW) = 3
fchdir(3) = 0
close(3) = 0
mkdir("d", 0777) = 0
...
Not sure why it uses open/fchdir for 'c' instead of just chdir; maybe that's to avoid a TOCTOU race that we may not care about.
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] env util: Implement CreateDirsRecursively()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: env_util: Implement CreateDirsRecursively()
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5618/1/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:
PS1, Line 199: dir_name.empty()
I don't think this can happen. Take the path "/foo/bar/baz". Successive DirName() calls will yield "/foo/bar", "/foo", and "/" (and then "/" over and over).
Line 202: for (auto path = paths.crbegin(); path != paths.crend(); ++path) {
Nit: I think it'd be clearer to explicitly reverse the vector (there's probably something in <algorithm> for that), then do a normal "for (auto path : paths)" loop.
http://gerrit.cloudera.org:8080/#/c/5618/1/src/kudu/util/env_util.h
File src/kudu/util/env_util.h:
Line 75: Status CreateDirsRecursively(Env* env, std::string path);
Pass 'path' by cref?
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add some path / env related helper functions
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add some path / env related helper functions
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5618
to look at the new patch set (#3).
Change subject: Add some path / env related helper functions
......................................................................
Add some path / env related helper functions
* path_util: Add SplitPath() helper function
This function provides a (theoretically) portable method of splitting
a filesystem path into its constituent components.
* env: Add GetCurrentWorkingDir() and ChangeDir()
These are functions that were missing from our Env implementation.
* env_util: Implement CreateDirsRecursively()
This function is similar in spirit to the "mkdir -p" command.
Also reordered a few includes and deleted a few lines of commented-out code.
Change-Id: Ia664708a09493923abbf2ff4e56e3d49c62cf97e
---
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/path_util-test.cc
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
9 files changed, 182 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/5618/3
--
To view, visit http://gerrit.cloudera.org:8080/5618
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia664708a09493923abbf2ff4e56e3d49c62cf97e
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] env util: Implement CreateDirsRecursively()
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: env_util: Implement CreateDirsRecursively()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5618/1/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:
Line 202: for (auto path = paths.crbegin(); path != paths.crend(); ++path) {
> I was trying to write it to abstract away the details of what a path is, an
I'm skeptical of the value of abstraction in this case since it obscures what's going on and makes the code harder to follow. Even in the case that we eventually ported to Windows at some point, it would be a relatively simple change to use '\' instead of '/' here.
If you really want to make the portability argument, I'd say introduce a constant 'kPathSeparator' in path_util.h which is set to '/' (analogous to python's os.path.sep). Or introduce a function SplitPath() in path_util. But repeatedly calling DirName like this is just more confusing IMO.
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] env util: Implement CreateDirsRecursively()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5618
to look at the new patch set (#2).
Change subject: env_util: Implement CreateDirsRecursively()
......................................................................
env_util: Implement CreateDirsRecursively()
This function is similar in spirit to the "mkdir -p" command.
Also reordered a few includes and deleted a few lines of commented-out code.
Change-Id: Ia664708a09493923abbf2ff4e56e3d49c62cf97e
---
M src/kudu/util/env_util-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
3 files changed, 80 insertions(+), 9 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/5618/2
--
To view, visit http://gerrit.cloudera.org:8080/5618
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia664708a09493923abbf2ff4e56e3d49c62cf97e
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] env util: Implement CreateDirsRecursively()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: env_util: Implement CreateDirsRecursively()
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5618/2/src/kudu/util/env_util-test.cc
File src/kudu/util/env_util-test.cc:
PS2, Line 90: JoinPathSegments(JoinPathSegments(JoinPathSegments(test_dir_, "x"), "y"), "z")
> again on the portability thing, I'd personally rather read "x/y/z" since we
Done
http://gerrit.cloudera.org:8080/#/c/5618/2/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:
PS2, Line 195: = ""
> isn't this redundant? the default constructor is already ""
Done
Line 200: if (s.ok() && is_dir) continue;
> If you're aiming for mkdir -p semantics, it should be ok for one of the pat
Well spotted. Rather than jumping around using chdir(), I used realpath() via Env::Canonicalize() when detecting that a file exists in a subpath but it's not a normal directory. I think that's probably alright because it would be rare to have a very long path composed entirely of symlinks.
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add some path / env related helper functions
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5618
to look at the new patch set (#4).
Change subject: Add some path / env related helper functions
......................................................................
Add some path / env related helper functions
* path_util: Add SplitPath() helper function
This function provides a (theoretically) portable method of splitting
a filesystem path into its constituent components.
* env: Add GetCurrentWorkingDir() and ChangeDir()
These are functions that were missing from our Env implementation.
* env_util: Implement CreateDirsRecursively()
This function is similar in spirit to the "mkdir -p" command.
Also reordered a few includes and deleted a few lines of commented-out code.
Change-Id: Ia664708a09493923abbf2ff4e56e3d49c62cf97e
---
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/path_util-test.cc
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
9 files changed, 176 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/5618/4
--
To view, visit http://gerrit.cloudera.org:8080/5618
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia664708a09493923abbf2ff4e56e3d49c62cf97e
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: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add some path / env related helper functions
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] env util: Implement CreateDirsRecursively()
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: env_util: Implement CreateDirsRecursively()
......................................................................
Patch Set 2:
(2 comments)
general note: while I like fine-grained patches, for these simple utility functions I typically find them easier to review when they come with their usage context (eg this could be merged with the chdir/cwd patch, the split paths, the isrelative, etc)
http://gerrit.cloudera.org:8080/#/c/5618/2/src/kudu/util/env_util-test.cc
File src/kudu/util/env_util-test.cc:
PS2, Line 90: JoinPathSegments(JoinPathSegments(JoinPathSegments(test_dir_, "x"), "y"), "z")
again on the portability thing, I'd personally rather read "x/y/z" since we are so far from windows support and it's hard to see what's going on here. (same above)
http://gerrit.cloudera.org:8080/#/c/5618/2/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:
PS2, Line 195: = ""
isn't this redundant? the default constructor is already ""
--
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: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add some path / env related helper functions
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Add some path / env related helper functions
......................................................................
Patch Set 4: Verified+1
Yeah, spurious failure.
--
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] env util: Implement CreateDirsRecursively()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: env_util: Implement CreateDirsRecursively()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5618/1/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:
Line 202: for (auto path = paths.crbegin(); path != paths.crend(); ++path) {
> I'm skeptical of the value of abstraction in this case since it obscures wh
I like the idea of a SplitPath() function. I'll go that route.
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add some path / env related helper functions
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.
Change subject: Add some path / env related helper functions
......................................................................
Add some path / env related helper functions
* path_util: Add SplitPath() helper function
This function provides a (theoretically) portable method of splitting
a filesystem path into its constituent components.
* env: Add GetCurrentWorkingDir() and ChangeDir()
These are functions that were missing from our Env implementation.
* env_util: Implement CreateDirsRecursively()
This function is similar in spirit to the "mkdir -p" command.
Also reordered a few includes and deleted a few lines of commented-out code.
Change-Id: Ia664708a09493923abbf2ff4e56e3d49c62cf97e
Reviewed-on: http://gerrit.cloudera.org:8080/5618
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Mike Percy <mp...@apache.org>
---
M src/kudu/util/env-test.cc
M src/kudu/util/env.h
M src/kudu/util/env_posix.cc
M src/kudu/util/env_util-test.cc
M src/kudu/util/env_util.cc
M src/kudu/util/env_util.h
M src/kudu/util/path_util-test.cc
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
9 files changed, 176 insertions(+), 11 deletions(-)
Approvals:
Mike Percy: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/5618
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia664708a09493923abbf2ff4e56e3d49c62cf97e
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Add some path / env related helper functions
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Add some path / env related helper functions
......................................................................
Patch Set 4: Code-Review+2
Test failure looks spurious.
--
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: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] env util: Implement CreateDirsRecursively()
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add some path / env related helper functions
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: Add some path / env related helper functions
......................................................................
Patch Set 3: -Code-Review
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5618/3/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 896: unique_ptr<char, FreeDeleter> wd(getcwd(NULL, 0));
> Please ask a Kudu macOS-based dev to build this before merging.
Done; compiles fine on macOS.
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Add some path / env related helper functions
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Add some path / env related helper functions
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5618/3/src/kudu/util/env_posix.cc
File src/kudu/util/env_posix.cc:
Line 896: unique_ptr<char, FreeDeleter> wd(getcwd(NULL, 0));
Please ask a Kudu macOS-based dev to build this before merging.
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?
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 m
Agreed with Dinesh; test_dir_ will be recursively cleaned up when the test finishes, and verifying DeleteRecursively() shouldn't be in scope for this test.
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?
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] env util: Implement CreateDirsRecursively()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: env_util: Implement CreateDirsRecursively()
......................................................................
Patch Set 1:
(6 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
We don't always have a guarantee, but on Jenkins it's ENV['TEST_TMPDIR'] which defaults to /tmp/kudutest-$uid/
I'll add a chdir()
Line 81: }
> how about some test cases for failure cases? mkdir on top of an existing fi
Good idea, I'll add those.
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 fi
Good catch. Will do.
PS1, Line 199: dir_name.empty()
> I don't think this can happen. Take the path "/foo/bar/baz". Successive Dir
Ah, you are right, dirname("foo") == ".". I'll remove this check.
Line 202: for (auto path = paths.crbegin(); path != paths.crend(); ++path) {
> Nit: I think it'd be clearer to explicitly reverse the vector (there's prob
I was trying to write it to abstract away the details of what a path is, and only focus on path components, to make it potentially more portable in the future.
http://gerrit.cloudera.org:8080/#/c/5618/1/src/kudu/util/env_util.h
File src/kudu/util/env_util.h:
Line 75: Status CreateDirsRecursively(Env* env, std::string path);
> Pass 'path' by cref?
I didn't do that because I'm reusing the variable in the function body and it gives the user the option of passing it in as a movable string.
--
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 <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Lars Volker <lv...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes