[kudu-CR] Give more context on errors reading cfiles

[Todd Lipcon (Code Review)]

Todd Lipcon has submitted this change and it was merged.

Change subject: Give more context on errors reading cfiles
..


Give more context on errors reading cfiles

Recently, a user reported an error loading a tablet in which the only
reported message was "bad magic". This wasn't useful for pinpointing the
id of the bad block or what might have happened to cause the problem.

This patch adds more context info in such circumstances: we now include
DebugString output for "bad magic" errors as well as the block ID in all
cases.

The unit test is updated to check that block IDs show up in all
Corruption status messages.

Change-Id: I0bda5688a020032c512235ee574cb3e53c7872af
Reviewed-on: http://gerrit.cloudera.org:8080/7620
Reviewed-by: Adar Dembo 
Tested-by: Kudu Jenkins
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/index_btree.cc
M src/kudu/tablet/deltafile.cc
4 files changed, 52 insertions(+), 19 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I0bda5688a020032c512235ee574cb3e53c7872af
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Give more context on errors reading cfiles

[Adar Dembo (Code Review)]

Adar Dembo has posted comments on this change.

Change subject: Give more context on errors reading cfiles
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0bda5688a020032c512235ee574cb3e53c7872af
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Give more context on errors reading cfiles

[Todd Lipcon (Code Review)]

Hello David Ribeiro Alves, Andrew Wong, Grant Henke, Kudu Jenkins,

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

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

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

Change subject: Give more context on errors reading cfiles
..

Give more context on errors reading cfiles

Recently, a user reported an error loading a tablet in which the only
reported message was "bad magic". This wasn't useful for pinpointing the
id of the bad block or what might have happened to cause the problem.

This patch adds more context info in such circumstances: we now include
DebugString output for "bad magic" errors as well as the block ID in all
cases.

The unit test is updated to check that block IDs show up in all
Corruption status messages.

Change-Id: I0bda5688a020032c512235ee574cb3e53c7872af
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/index_btree.cc
M src/kudu/tablet/deltafile.cc
4 files changed, 52 insertions(+), 19 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0bda5688a020032c512235ee574cb3e53c7872af
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] Give more context on errors reading cfiles

[Andrew Wong (Code Review)]

Andrew Wong has posted comments on this change.

Change subject: Give more context on errors reading cfiles
..


Patch Set 1: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7620/1/src/kudu/cfile/cfile_reader.cc
File src/kudu/cfile/cfile_reader.cc:

Line 196: return Status::Corruption("invalid CFile header size", 
std::to_string(header_size));
> The block id doesn't need to be here in the ReadAndParseHeader code because
+1 on the data dir (fine with it being in a separate patch)

FWIW the HandleError() calls in file_block_manager.cc and log_block_manager.cc 
have the, albeit minor, "plumbing"


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0bda5688a020032c512235ee574cb3e53c7872af
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Give more context on errors reading cfiles

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Give more context on errors reading cfiles
..


Patch Set 1: Code-Review+2

sounds good

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0bda5688a020032c512235ee574cb3e53c7872af
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] Give more context on errors reading cfiles

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Give more context on errors reading cfiles
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7620/1/src/kudu/cfile/cfile_reader.cc
File src/kudu/cfile/cfile_reader.cc:

Line 196: return Status::Corruption("invalid CFile header size", 
std::to_string(header_size));
> would it be possible to display the data directory? this might hint at a fa
The block id doesn't need to be here in the ReadAndParseHeader code because 
this is only called by Init() which includes the block id.

Including the data dir is a nice idea. Mind if I postpone that to a separate 
patch since I think it's a bit more complicated? (have to add a new method to 
ReadableBlock and perhaps do a bit of plumbing)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0bda5688a020032c512235ee574cb3e53c7872af
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] Give more context on errors reading cfiles

[David Ribeiro Alves (Code Review)]

David Ribeiro Alves has posted comments on this change.

Change subject: Give more context on errors reading cfiles
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7620/1/src/kudu/cfile/cfile_reader.cc
File src/kudu/cfile/cfile_reader.cc:

Line 196: return Status::Corruption("invalid CFile header size", 
std::to_string(header_size));
> Should the block_id be given here too? (and other similar size/parse checks
would it be possible to display the data directory? this might hint at a 
failing disk so it would be good to provide a hint at which disk to check on.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0bda5688a020032c512235ee574cb3e53c7872af
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] Give more context on errors reading cfiles

[Grant Henke (Code Review)]

Grant Henke has posted comments on this change.

Change subject: Give more context on errors reading cfiles
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7620/1/src/kudu/cfile/cfile-test.cc
File src/kudu/cfile/cfile-test.cc:

Line 852:   ASSERT_STR_MATCHES(s.ToString(), "block [0-9]+");
This test doesn't necessarily validate every error message. We corrupt each 
bit, but not necessarily to trigger ever "kind" of failure. I don't think a 
test like that is reasonable or worth it. But I thought I would mention it.


http://gerrit.cloudera.org:8080/#/c/7620/1/src/kudu/cfile/cfile_reader.cc
File src/kudu/cfile/cfile_reader.cc:

Line 196: return Status::Corruption("invalid CFile header size", 
std::to_string(header_size));
Should the block_id be given here too? (and other similar size/parse checks)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I0bda5688a020032c512235ee574cb3e53c7872af
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] Give more context on errors reading cfiles

[Todd Lipcon (Code Review)]

Hello David Ribeiro Alves, Grant Henke, Andrew Wong,

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

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

to review the following change.

Change subject: Give more context on errors reading cfiles
..

Give more context on errors reading cfiles

Recently, a user reported an error loading a tablet in which the only
reported message was "bad magic". This wasn't useful for pinpointing the
id of the bad block or what might have happened to cause the problem.

This patch adds more context info in such circumstances: we now include
DebugString output for "bad magic" errors as well as the block ID in all
cases.

The unit test is updated to check that block IDs show up in all
Corruption status messages.

Change-Id: I0bda5688a020032c512235ee574cb3e53c7872af
---
M src/kudu/cfile/cfile-test.cc
M src/kudu/cfile/cfile_reader.cc
M src/kudu/cfile/index_btree.cc
3 files changed, 37 insertions(+), 15 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I0bda5688a020032c512235ee574cb3e53c7872af
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon 
Gerrit-Reviewer: Andrew Wong 
Gerrit-Reviewer: David Ribeiro Alves 
Gerrit-Reviewer: Grant Henke