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

[Dan Burkert (Code Review)]

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 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins

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

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

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

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

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

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins

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

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

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

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins

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

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

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

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

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

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

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

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins

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

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

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

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Dan Burkert 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

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

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

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

[Dan Burkert (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo