You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Maxim Smyatkin (Code Review)" <ge...@cloudera.org> on 2016/11/08 20:30:02 UTC
[kudu-CR] KUDU-1634. TS and MS should delete tmp metadata files on startup
Maxim Smyatkin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5007
Change subject: KUDU-1634. TS and MS should delete tmp metadata files on startup
......................................................................
KUDU-1634. TS and MS should delete tmp metadata files on startup
Initially it was suggested that TS should delete tmp files from consensus-meta
directory, but these leftovers can appear anywhere where atomic write with
WritePBContainerToPath() is being used. For now, it is consensus metadata,
black manager's data directory, instance metadata, etc. Also, it happens to
Master server as well.
Hence, I've decided to deal with it on FsManager level and scan all root paths
for tmp files. It may slow startup a little bit, but I don't know any way
around it because:
a) it can't be done on background as it can remove the real tmp files
this way;
b) it would be nice to have a separate tmp directory, but it makes rename not
atomic this way (because we can - and usually will - have root/wal dirs on
different drives).
Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
---
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
3 files changed, 95 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/5007/1
--
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1634. TS and master should delete tmp metadata files on startup
......................................................................
Patch Set 5: Code-Review+2
Looks good, though it would be nice if Todd gave his approval to the overall approach since he originally filed the bug.
--
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-HasComments: No
[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1634. TS and master should delete tmp metadata files on startup
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5007/3/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
Line 224: TEST_F(FsManagerTestBase, TestTmpFilesCleanup) {
Could you add something to the test file tree to exercise the checked_dirs logic? Maybe a self-referential symlink or something that would cause an infinite loop otherwise?
http://gerrit.cloudera.org:8080/#/c/5007/3/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
Line 497: if (checked_dirs.find(canonized_path) == checked_dirs.end()) {
Use ContainsKey() from gutil/map-util.h.
--
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup
Posted by "Maxim Smyatkin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5007
to look at the new patch set (#5).
Change subject: KUDU-1634. TS and master should delete tmp metadata files on startup
......................................................................
KUDU-1634. TS and master should delete tmp metadata files on startup
Initially it was suggested that TS should delete tmp files from consensus-meta
directory, but these leftovers can appear anywhere where atomic write with
WritePBContainerToPath() is being used. For now, it is consensus metadata,
block manager's data directory, instance metadata, etc. Also, it happens to
Master server as well.
Hence, I've decided to deal with it on FsManager level and scan all root paths
for tmp files. It may slow startup a little bit, but I don't know any way
around it because:
a) it can't be done on background as it can remove the real tmp files
this way;
b) it would be nice to have a separate tmp directory, but it makes rename not
atomic this way (because we can - and usually will - have root/wal dirs on
different drives).
Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
---
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
3 files changed, 154 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/5007/5
--
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1634. TS and master should delete tmp metadata files on startup
......................................................................
Patch Set 5:
Are you guys worried about a user somehow accidentally having a link from their data dir out to something like / ? I'm not sure there's any real good reason why that would happen, but makes me just a little nervous.
--
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1634. TS and master should delete tmp metadata files on startup
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5007/3/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
Line 224: TEST_F(FsManagerTestBase, TestTmpFilesCleanup) {
> Sure, I can. But how will Jenkins behave if it actually does loop forever?
All unit tests are given up to 900s to execute; if they take longer, they are forcefully aborted.
--
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1634. TS and master should delete tmp metadata files on startup
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5007/4/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
Line 268: ASSERT_OK(CountTmpFiles(fs_manager()->env(), lookup_dirs, &checked_dirs, &n_tmp_files));
Seems like checked_dirs could be local to CountTmpFiles, and doesn't need to leak out into the test itself (since the test is only clearing it).
--
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup
Posted by "Maxim Smyatkin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5007
to look at the new patch set (#3).
Change subject: KUDU-1634. TS and master should delete tmp metadata files on startup
......................................................................
KUDU-1634. TS and master should delete tmp metadata files on startup
Initially it was suggested that TS should delete tmp files from consensus-meta
directory, but these leftovers can appear anywhere where atomic write with
WritePBContainerToPath() is being used. For now, it is consensus metadata,
block manager's data directory, instance metadata, etc. Also, it happens to
Master server as well.
Hence, I've decided to deal with it on FsManager level and scan all root paths
for tmp files. It may slow startup a little bit, but I don't know any way
around it because:
a) it can't be done on background as it can remove the real tmp files
this way;
b) it would be nice to have a separate tmp directory, but it makes rename not
atomic this way (because we can - and usually will - have root/wal dirs on
different drives).
Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
---
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
3 files changed, 138 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/5007/3
--
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: KUDU-1634. TS and master should delete tmp metadata files on startup
......................................................................
KUDU-1634. TS and master should delete tmp metadata files on startup
Initially it was suggested that TS should delete tmp files from consensus-meta
directory, but these leftovers can appear anywhere where atomic write with
WritePBContainerToPath() is being used. For now, it is consensus metadata,
block manager's data directory, instance metadata, etc. Also, it happens to
Master server as well.
Hence, I've decided to deal with it on FsManager level and scan all root paths
for tmp files. It may slow startup a little bit, but I don't know any way
around it because:
a) it can't be done on background as it can remove the real tmp files
this way;
b) it would be nice to have a separate tmp directory, but it makes rename not
atomic this way (because we can - and usually will - have root/wal dirs on
different drives).
Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Reviewed-on: http://gerrit.cloudera.org:8080/5007
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
3 files changed, 154 insertions(+), 6 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup
Posted by "Maxim Smyatkin (Code Review)" <ge...@cloudera.org>.
Maxim Smyatkin has posted comments on this change.
Change subject: KUDU-1634. TS and master should delete tmp metadata files on startup
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5007/3/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
Line 224: TEST_F(FsManagerTestBase, TestTmpFilesCleanup) {
> Could you add something to the test file tree to exercise the checked_dirs
Sure, I can. But how will Jenkins behave if it actually does loop forever? Will it stop it and report a failure?
--
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1634. TS and master should delete tmp metadata files on startup
......................................................................
Patch Set 5:
FYI, Maxim is working on a change to the infix in a follow-on patch so I'm going to merge this one as-is.
--
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1634. TS and master should delete tmp metadata files on startup
......................................................................
Patch Set 5:
> Are you guys worried about a user somehow accidentally having a
> link from their data dir out to something like / ? I'm not sure
> there's any real good reason why that would happen, but makes me
> just a little nervous.
Then everything in / owned by the Kudu user will be deleted.
We can avoid that by not following symlinks, or at least not following them out of the data directories (i.e. symlinks that point to stuff within the data dirs could be followed). Though didn't you use a symlink once to redirect Kudu's placement of files (maybe cmeta files?) from one disk to another? I guess that use case wasn't for data directories.
--
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1634. TS and master should delete tmp metadata files on startup
......................................................................
Patch Set 5:
Yea, I guess if there's a symlink for the metadata we should follow it, for the use case you mentioned. I guess we'll cross our fingers.
Maybe we should consider changing the tmp infix to 'kudutmp' or something so it's less likely to do any real damage if it does escape?
--
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup
Posted by "Maxim Smyatkin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5007
to look at the new patch set (#2).
Change subject: KUDU-1634. TS and master should delete tmp metadata files on startup
......................................................................
KUDU-1634. TS and master should delete tmp metadata files on startup
Initially it was suggested that TS should delete tmp files from consensus-meta
directory, but these leftovers can appear anywhere where atomic write with
WritePBContainerToPath() is being used. For now, it is consensus metadata,
block manager's data directory, instance metadata, etc. Also, it happens to
Master server as well.
Hence, I've decided to deal with it on FsManager level and scan all root paths
for tmp files. It may slow startup a little bit, but I don't know any way
around it because:
a) it can't be done on background as it can remove the real tmp files
this way;
b) it would be nice to have a separate tmp directory, but it makes rename not
atomic this way (because we can - and usually will - have root/wal dirs on
different drives).
Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
---
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
3 files changed, 138 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/5007/2
--
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup
Posted by "Maxim Smyatkin (Code Review)" <ge...@cloudera.org>.
Maxim Smyatkin has posted comments on this change.
Change subject: KUDU-1634. TS and master should delete tmp metadata files on startup
......................................................................
Patch Set 5:
Having more specific tmp infix seems good.
--
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup
Posted by "Maxim Smyatkin (Code Review)" <ge...@cloudera.org>.
Maxim Smyatkin has posted comments on this change.
Change subject: KUDU-1634. TS and master should delete tmp metadata files on startup
......................................................................
Patch Set 5:
I guess it would need Kudu servers to be executed by someone like root and it will only remove .tmp files in this case.
But it's your call :) I suppose we can check relative paths, so that symlink doesn't go anywhere outside kudu fs root paths, if it's really an issue.
--
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1634. TS and master should delete tmp metadata files on startup
Posted by "Maxim Smyatkin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5007
to look at the new patch set (#4).
Change subject: KUDU-1634. TS and master should delete tmp metadata files on startup
......................................................................
KUDU-1634. TS and master should delete tmp metadata files on startup
Initially it was suggested that TS should delete tmp files from consensus-meta
directory, but these leftovers can appear anywhere where atomic write with
WritePBContainerToPath() is being used. For now, it is consensus metadata,
block manager's data directory, instance metadata, etc. Also, it happens to
Master server as well.
Hence, I've decided to deal with it on FsManager level and scan all root paths
for tmp files. It may slow startup a little bit, but I don't know any way
around it because:
a) it can't be done on background as it can remove the real tmp files
this way;
b) it would be nice to have a separate tmp directory, but it makes rename not
atomic this way (because we can - and usually will - have root/wal dirs on
different drives).
Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
---
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/fs/fs_manager.h
3 files changed, 156 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/5007/4
--
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Maxim Smyatkin <sm...@gmail.com>
[kudu-CR] KUDU-1634. TS and MS should delete tmp metadata files on startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1634. TS and MS should delete tmp metadata files on startup
......................................................................
Patch Set 1:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/5007/1//COMMIT_MSG
Commit Message:
PS1, Line 7: MS
I presume this means 'master'? Could you use 'master' instead?
PS1, Line 12: black
block
http://gerrit.cloudera.org:8080/#/c/5007/1/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
Line 199: std::vector<string> sub_objects;
Nit: add using and drop the prefix.
PS1, Line 207: ".tmp"
Let's expose this from within FsManager so that we've got it defined in just one place.
Line 224: env_util::OpenFileForWrite(fs_manager()->env(), tmp_path, &tmp_writer);
Should ASSERT_OK() on anything here that returns a Status.
Line 248: ASSERT_EQ(n_tmp_files, 4);
Nit: with ASSERT_EQ(), we typically place the expected value before the actual value. So this would be ASSERT_EQ(4, n_tmp_files).
PS1, Line 253: n_tmp_files = 0;
: for (const string& root : lookup_dirs) {
: vector<string> children;
: ASSERT_OK(fs_manager()->env()->GetChildren(root, &children));
: n_tmp_files += CountTmpFiles(fs_manager()->env(), root, children);
: }
: ASSERT_EQ(n_tmp_files, 0);
Looks like all of this could be encapsulated in a second method that calls into CountTmpFiles().
http://gerrit.cloudera.org:8080/#/c/5007/1/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
Line 230: // Remove leftover tmp files
This behavior should be conditioned on !read_only_.
Line 231: std::vector<string> children;
Nit: replace std::vector with vector and add a "using std::vector;" statement towards the top of the file.
Line 460: void FsManager::CleanTmpFiles(const string& path, const std::vector<string>& children) {
Nit: remove std:: prefix here and below too.
Line 463:
Please add a DCHECK(!read_only_) here.
Line 468: Status s = env_->GetChildren(sub_path, &sub_objects);
I see you're relying on GetChildren() to also tell you whether you're trying to recurse on a file or a directory (it'll fail if the former). Is that strictly better than explicitly checking whether a child entry is a directory/symlink_to_directory?
The advantage of an explicit check is that you can then treat GetChildren() failures as real failures, and LOG(WARNING) them.
Line 470: CleanTmpFiles(sub_path, sub_objects);
I'd prefer if you didn't use functional recursion, to avoid blowing out the thread's stack in the event of a very large tree.
Also, can you make sure this doesn't loop forever in the event of a recursive symlink? You may need to keep track of paths you've cleaned to do that.
http://gerrit.cloudera.org:8080/#/c/5007/1/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
Line 241: void CleanTmpFiles(const std::string& path, const std::vector<std::string>& children);
Please add a comment explaining what this does.
--
To view, visit http://gerrit.cloudera.org:8080/5007
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I299e7b22c3f920430ff5d581cf7a507a83eb0101
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Maxim Smyatkin <sm...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes