You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Grant Henke (Code Review)" <ge...@cloudera.org> on 2018/08/17 19:34:02 UTC

[kudu-CR] KUDU-2531: (part 1) Ignore backup files 

Grant Henke has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11259


Change subject: KUDU-2531: (part 1) Ignore backup files
......................................................................

KUDU-2531: (part 1) Ignore backup files

Backup files may be left behind by tools and will prevent the server from starting. This patch ensures files with a .bak extension are ignored.

Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
---
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/util/env_util.cc
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
6 files changed, 17 insertions(+), 2 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11259/1
-- 
To view, visit http://gerrit.cloudera.org:8080/11259
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Gerrit-Change-Number: 11259
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>

[kudu-CR] KUDU-2531: (part 1) Ignore backup files

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Attila Bukor, Kudu Jenkins, Andrew Wong, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/11259

to look at the new patch set (#2).

Change subject: KUDU-2531: (part 1) Ignore backup files
......................................................................

KUDU-2531: (part 1) Ignore backup files

Backup files may be left behind by tools and will prevent the server from starting. This patch ensures files with a .bak extension are ignored.

Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
---
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
M src/kudu/tools/tool_action_pbc.cc
M src/kudu/util/path_util.cc
M src/kudu/util/path_util.h
5 files changed, 13 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11259/2
-- 
To view, visit http://gerrit.cloudera.org:8080/11259
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Gerrit-Change-Number: 11259
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2531: (part 1) Ignore invalid tablet metadata files

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11259 )

Change subject: KUDU-2531: (part 1) Ignore invalid tablet metadata files
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11259/4/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/11259/4/src/kudu/fs/fs_manager.cc@633
PS4, Line 633:   ObjectIdGenerator oid_generator;
Creating a new one of these for every call seems wasteful when we can have thousands of tablets. Can you store one in the FsManager itself?



-- 
To view, visit http://gerrit.cloudera.org:8080/11259
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Gerrit-Change-Number: 11259
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 20 Aug 2018 19:36:30 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2531: (part 1) Ignore backup files

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11259 )

Change subject: KUDU-2531: (part 1) Ignore backup files
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11259/1/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:

http://gerrit.cloudera.org:8080/#/c/11259/1/src/kudu/util/env_util.cc@312
PS1, Line 312:         iter->find(kTmpInfix) != string::npos ||
             :         iter->find(kBakInfix) != string::npos) {
> It's a good question. I followed the places kTmpInfix was ignored assuming 
If it's useful for tests, we should keep it, but we should move the function into a test util module (and maybe rename it) so that this behavior is more clear.



-- 
To view, visit http://gerrit.cloudera.org:8080/11259
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Gerrit-Change-Number: 11259
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 17 Aug 2018 21:43:22 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2531: (part 1) Ignore invalid tablet metadata files

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11259 )

Change subject: KUDU-2531: (part 1) Ignore invalid tablet metadata files
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11259/4/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/11259/4/src/kudu/fs/fs_manager.cc@633
PS4, Line 633:   ObjectIdGenerator oid_generator;
> Creating a new one of these for every call seems wasteful when we can have 
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/11259
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Gerrit-Change-Number: 11259
Gerrit-PatchSet: 4
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 20 Aug 2018 19:51:12 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2531: (part 1) Ignore invalid tablet metadata files

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Attila Bukor, Kudu Jenkins, Andrew Wong, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/11259

to look at the new patch set (#6).

Change subject: KUDU-2531: (part 1) Ignore invalid tablet metadata files
......................................................................

KUDU-2531: (part 1) Ignore invalid tablet metadata files

Changes the behavior of FsManager::IsValidTabletId
to return false for all files that are named with invalid
tabet ids. This results in all files that are not valid
table ids being ignored, instead of just tmp files.

Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
---
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, 37 insertions(+), 16 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11259/6
-- 
To view, visit http://gerrit.cloudera.org:8080/11259
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Gerrit-Change-Number: 11259
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2531: (part 1) Ignore backup files

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11259 )

Change subject: KUDU-2531: (part 1) Ignore backup files
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11259/1/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:

http://gerrit.cloudera.org:8080/#/c/11259/1/src/kudu/util/env_util.cc@312
PS1, Line 312:         iter->find(kTmpInfix) != string::npos ||
             :         iter->find(kBakInfix) != string::npos) {
> Both of these exceptions feel really strange to me. These are real files; w
It's a good question. I followed the places kTmpInfix was ignored assuming backup files would want to be too.

It looks like this was moved from ExternalMiniClusterFsInspector at one point: https://github.com/apache/kudu/commit/42711bac4017b3ad11ce4d20ff9e61b6eb0d69f9

The commit that added the tmp filtering is here:
https://github.com/apache/kudu/commit/8186cc18db6f2879e6148ab1eff3a30f272e3e83

Even now it looks like that call is only used in tests. It looks like I could safely leave this out from there if you like. I can doc that .tmp files are ignored too.



-- 
To view, visit http://gerrit.cloudera.org:8080/11259
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Gerrit-Change-Number: 11259
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 17 Aug 2018 21:30:08 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2531: (part 1) Ignore backup files

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11259 )

Change subject: KUDU-2531: (part 1) Ignore backup files
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11259/1/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/11259/1/src/kudu/fs/fs_manager.cc@625
PS1, Line 625: // Return true if 'fname' is a valid tablet ID.
Any idea why we don't implement this by verifying that the filename is a 32 character hexadecimal sequence? Not sure why we need all these special cases.


http://gerrit.cloudera.org:8080/#/c/11259/1/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:

http://gerrit.cloudera.org:8080/#/c/11259/1/src/kudu/util/env_util.cc@312
PS1, Line 312:         iter->find(kTmpInfix) != string::npos ||
             :         iter->find(kBakInfix) != string::npos) {
Both of these exceptions feel really strange to me. These are real files; why would we ignore them in a ListFiles function?



-- 
To view, visit http://gerrit.cloudera.org:8080/11259
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Gerrit-Change-Number: 11259
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 17 Aug 2018 21:07:15 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2531: (part 1) Ignore backup files

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11259 )

Change subject: KUDU-2531: (part 1) Ignore backup files
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11259/1/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:

http://gerrit.cloudera.org:8080/#/c/11259/1/src/kudu/util/env_util.cc@312
PS1, Line 312:         iter->find(kTmpInfix) != string::npos ||
             :         iter->find(kBakInfix) != string::npos) {
> If it's useful for tests, we should keep it, but we should move the functio
There are no tests today that will make a backup file. As far as temp files go, I agree.



-- 
To view, visit http://gerrit.cloudera.org:8080/11259
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Gerrit-Change-Number: 11259
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Fri, 17 Aug 2018 21:49:25 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2531: (part 1) Ignore backup files

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/11259 )

Change subject: KUDU-2531: (part 1) Ignore backup files
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/11259/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11259/2//COMMIT_MSG@9
PS2, Line 9: Backup files may be left behind by tools and will prevent the server from starting. This patch ensures files with a .bak extension are ignored.
nit: can you break this into shorter lines?


http://gerrit.cloudera.org:8080/#/c/11259/1/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/11259/1/src/kudu/fs/fs_manager.cc@625
PS1, Line 625: // Return true if 'fname' is a valid tablet ID.
> Any idea why we don't implement this by verifying that the filename is a 32
yeah, I agree it would be a better approach



-- 
To view, visit http://gerrit.cloudera.org:8080/11259
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Gerrit-Change-Number: 11259
Gerrit-PatchSet: 2
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Sat, 18 Aug 2018 14:07:47 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2531: (part 1) Ignore invalid tablet metadata files

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11259 )

Change subject: KUDU-2531: (part 1) Ignore invalid tablet metadata files
......................................................................


Patch Set 6: Code-Review+2

Carrying the +2 through the rebase and lint fix.


-- 
To view, visit http://gerrit.cloudera.org:8080/11259
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Gerrit-Change-Number: 11259
Gerrit-PatchSet: 6
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Tue, 21 Aug 2018 03:31:25 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2531: (part 1) Ignore invalid tablet metadata files

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Attila Bukor, Kudu Jenkins, Andrew Wong, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/11259

to look at the new patch set (#5).

Change subject: KUDU-2531: (part 1) Ignore invalid tablet metadata files
......................................................................

KUDU-2531: (part 1) Ignore invalid tablet metadata files

Changes the behavior of FsManager::IsValidTabletId
to return false for all files that are named with invalid
tabet ids. This results in all files that are not valid
table ids being ignored, instead of just tmp files.

Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
---
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, 36 insertions(+), 15 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11259/5
-- 
To view, visit http://gerrit.cloudera.org:8080/11259
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Gerrit-Change-Number: 11259
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2531: (part 1) Ignore invalid tablet metadata files

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Hello Attila Bukor, Kudu Jenkins, Andrew Wong, Adar Dembo, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/11259

to look at the new patch set (#3).

Change subject: KUDU-2531: (part 1) Ignore invalid tablet metadata files
......................................................................

KUDU-2531: (part 1) Ignore invalid tablet metadata files

Changes the behavior of FsManager::IsValidTabletId
to return false for all files that are named with invalid
tabet ids. This results in all files that are not valid
table ids being ignored, instead of just tmp files.

Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
---
M src/kudu/fs/fs_manager-test.cc
M src/kudu/fs/fs_manager.cc
2 files changed, 27 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/11259/3
-- 
To view, visit http://gerrit.cloudera.org:8080/11259
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Gerrit-Change-Number: 11259
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2531: (part 1) Ignore invalid tablet metadata files

Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/11259 )

Change subject: KUDU-2531: (part 1) Ignore invalid tablet metadata files
......................................................................


Patch Set 5: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/11259
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Gerrit-Change-Number: 11259
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 20 Aug 2018 22:30:08 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2531: (part 1) Ignore invalid tablet metadata files

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11259 )

Change subject: KUDU-2531: (part 1) Ignore invalid tablet metadata files
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/11259/1/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:

http://gerrit.cloudera.org:8080/#/c/11259/1/src/kudu/fs/fs_manager.cc@625
PS1, Line 625: // Return true if 'fname' is a valid tablet ID.
> yeah, I agree it would be a better approach
I can update this method to use ObjectIdGenerator::Canonicalize to ensure the ids are valid. Currently we have only filtered theses special file types. This will technically be more strict. Is there a risk of breaking anything?


http://gerrit.cloudera.org:8080/#/c/11259/1/src/kudu/fs/fs_manager.cc@625
PS1, Line 625: // Return true if 'fname' is a valid tablet ID.
> Any idea why we don't implement this by verifying that the filename is a 32
I am not sure why. It was added here: https://github.com/apache/kudu/commit/dc20efa0b66c798f73aafdc694a3566317366fd6

I think the implementation is assuming everything but these exceptions must be valid. I agree, it would be better to filter on the real pattern.


http://gerrit.cloudera.org:8080/#/c/11259/1/src/kudu/util/env_util.cc
File src/kudu/util/env_util.cc:

http://gerrit.cloudera.org:8080/#/c/11259/1/src/kudu/util/env_util.cc@312
PS1, Line 312:         iter->find(kTmpInfix) != string::npos ||
             :         iter->find(kBakInfix) != string::npos) {
> There are no tests today that will make a backup file. As far as temp files
I removed this change from the patch.



-- 
To view, visit http://gerrit.cloudera.org:8080/11259
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Gerrit-Change-Number: 11259
Gerrit-PatchSet: 1
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 20 Aug 2018 16:13:00 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2531: (part 1) Ignore invalid tablet metadata files

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11259 )

Change subject: KUDU-2531: (part 1) Ignore invalid tablet metadata files
......................................................................

KUDU-2531: (part 1) Ignore invalid tablet metadata files

Changes the behavior of FsManager::IsValidTabletId
to return false for all files that are named with invalid
tabet ids. This results in all files that are not valid
table ids being ignored, instead of just tmp files.

Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Reviewed-on: http://gerrit.cloudera.org:8080/11259
Tested-by: Kudu Jenkins
Reviewed-by: Grant Henke <gr...@apache.org>
---
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, 37 insertions(+), 16 deletions(-)

Approvals:
  Kudu Jenkins: Verified
  Grant Henke: Looks good to me, approved

-- 
To view, visit http://gerrit.cloudera.org:8080/11259
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Gerrit-Change-Number: 11259
Gerrit-PatchSet: 7
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-2531: (part 1) Ignore invalid tablet metadata files

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11259 )

Change subject: KUDU-2531: (part 1) Ignore invalid tablet metadata files
......................................................................


Patch Set 5: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/11259
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Gerrit-Change-Number: 11259
Gerrit-PatchSet: 5
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 20 Aug 2018 21:34:02 +0000
Gerrit-HasComments: No

[kudu-CR] KUDU-2531: (part 1) Ignore invalid tablet metadata files

Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11259 )

Change subject: KUDU-2531: (part 1) Ignore invalid tablet metadata files
......................................................................


Patch Set 3:

It turns out we use non canonical tablet ids all over our tests. Things like "test-tablet". So changing FsManager::IsValidTabletId to actually validate tablet ids breaks a lot.


-- 
To view, visit http://gerrit.cloudera.org:8080/11259
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If9f0e6dd8925519aa3ed3e735f2987743f2ef1d1
Gerrit-Change-Number: 11259
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Mon, 20 Aug 2018 17:15:05 +0000
Gerrit-HasComments: No