You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2017/04/07 15:56:59 UTC

[kudu-CR] KUDU-1966: Data directories can be removed erroneously 

Hello Adar Dembo,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: KUDU-1966: Data directories can be removed erroneously
......................................................................

KUDU-1966: Data directories can be removed erroneously

Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
---
M src/kudu/fs/block_manager_util-test.cc
M src/kudu/fs/block_manager_util.cc
2 files changed, 14 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: KUDU-1966: Data directories can be removed erroneously
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6589/2/src/kudu/fs/block_manager_util.cc
File src/kudu/fs/block_manager_util.cc:

Line 164:       // Check that all of the expected uuids are present.
There's something that still seems unintuitive about this check to me. Maybe it's because it's inside the loop (which is weird because this should only happen once), or because it's comparing container sizes instead of contents.

What if, outside the loop, we compared first_all_uuids.first with JoinStrings(uuids.keys, ",")? I think I'd find that more intuitive; do you as well?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: KUDU-1966: Data directories can be removed erroneously
......................................................................


Patch Set 1:

(1 comment)

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

Line 164:       // Check that all of the expected paths are present.
> My thought was that all of the error messages in this method are unintuitiv
I can buy that, but the errors (and comments) should at least be consistent with one another, and right now this one stands out from the others.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: KUDU-1966: Data directories can be removed erroneously
......................................................................


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1966: Data directories can be removed erroneously
......................................................................

KUDU-1966: Data directories can be removed erroneously

Also revises the error messages in
PathInstanceMetadataFile::CheckIntegrity to be in terms of data
directories, since this is what end-users will be familiar with. These
errors should be rare, but they can happen if a user is monkeying around
with data dirs configs.

Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
---
M src/kudu/fs/block_manager_util-test.cc
M src/kudu/fs/block_manager_util.cc
2 files changed, 60 insertions(+), 34 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/6589/4
-- 
To view, visit http://gerrit.cloudera.org:8080/6589
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: KUDU-1966: Data directories can be removed erroneously
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6589/2/src/kudu/fs/block_manager_util.cc
File src/kudu/fs/block_manager_util.cc:

Line 164:       // Check that all of the expected uuids are present.
> There's something that still seems unintuitive about this check to me. Mayb
The way I've structured it is the least invasive in terms of code changes and runtime overhead.  If I were going to do something like that I'd be more inclined to restructure the method pretty significantly.  Given the poor error messages, I think I'll just go ahead with that.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: KUDU-1966: Data directories can be removed erroneously
......................................................................


Patch Set 1:

(1 comment)

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

Line 164:       // Check that all of the expected paths are present.
> I can buy that, but the errors (and comments) should at least be consistent
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: KUDU-1966: Data directories can be removed erroneously
......................................................................


Patch Set 1:

(1 comment)

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

Line 164:       // Check that all of the expected paths are present.
> It's a little unintuitive to talk about paths here and not uuids; maybe "Ch
My thought was that all of the error messages in this method are unintuitive, since the user is specifying data directories, and the error messages are talking about UUIDs, which is entirely an internal detail.  I think UUIDs are even less clear than paths in the error message, so I'm a little bit reluctant to change it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: KUDU-1966: Data directories can be removed erroneously
......................................................................


Patch Set 1:

(1 comment)

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

Line 164:       // Check that all of the expected paths are present.
It's a little unintuitive to talk about paths here and not uuids; maybe "Check that the number of instance files is the same as the number of UUIDs"?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged.

Change subject: KUDU-1966: Data directories can be removed erroneously
......................................................................


KUDU-1966: Data directories can be removed erroneously

Also revises the error messages in
PathInstanceMetadataFile::CheckIntegrity to be in terms of data
directories, since this is what end-users will be familiar with. These
errors should be rare, but they can happen if a user is monkeying around
with data dirs configs.

Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
Reviewed-on: http://gerrit.cloudera.org:8080/6589
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/fs/block_manager_util-test.cc
M src/kudu/fs/block_manager_util.cc
2 files changed, 60 insertions(+), 34 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1966: Data directories can be removed erroneously
......................................................................

KUDU-1966: Data directories can be removed erroneously

Also revises the error messages in
PathInstanceMetadataFile::CheckIntegrity to be in terms of data
directories, since this is what end-users will be familiar with. These
errors should be rare, but they can happen if a user is monkeying around
with data dirs configs.

Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
---
M src/kudu/fs/block_manager_util-test.cc
M src/kudu/fs/block_manager_util.cc
2 files changed, 60 insertions(+), 34 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: KUDU-1966: Data directories can be removed erroneously
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6589/3/src/kudu/fs/block_manager_util.cc
File src/kudu/fs/block_manager_util.cc:

PS3, Line 129: level 
> level of
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: KUDU-1966: Data directories can be removed erroneously
......................................................................


Patch Set 3:

(1 comment)

Much better.

http://gerrit.cloudera.org:8080/#/c/6589/3/src/kudu/fs/block_manager_util.cc
File src/kudu/fs/block_manager_util.cc:

PS3, Line 129: level 
level of


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1966: Data directories can be removed erroneously

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: KUDU-1966: Data directories can be removed erroneously
......................................................................

KUDU-1966: Data directories can be removed erroneously

Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
---
M src/kudu/fs/block_manager_util-test.cc
M src/kudu/fs/block_manager_util.cc
2 files changed, 14 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1565069a3a596ddb41507dd95dd93dc85e021fbd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins