Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10505 )
Change subject: KUDU-702 Add block IDs to more log messages ...................................................................... Patch Set 1: (4 comments) Thanks for the patch! See inline for comments. Could use more detail in the commit message as well as a comment about how this was tested. For log messages, manual testing by running some of the tests and manually checking that the log messages were printed as expected is usually how we do it. Maybe log_block_manager-test would be a good test to check for the messages. http://gerrit.cloudera.org:8080/#/c/10505/1/src/kudu/cfile/cfile_reader.cc File src/kudu/cfile/cfile_reader.cc: http://gerrit.cloudera.org:8080/#/c/10505/1/src/kudu/cfile/cfile_reader.cc@491 PS1, Line 491: LOG(WARNING) << "Unable to validate compressed block at " : << ptr.offset() << " with id " << block_id().ToString() : << " of size " << block.size() << ": " : << s.ToString(); Nit: the ordering here seems off to me, I think we should first identify the block id and then print the offset and size, since offset and size are logically the two parts you need for a pointer. Same with the below change around line 510. http://gerrit.cloudera.org:8080/#/c/10505/1/src/kudu/cfile/index_btree.cc File src/kudu/cfile/index_btree.cc: http://gerrit.cloudera.org:8080/#/c/10505/1/src/kudu/cfile/index_btree.cc@109 PS1, Line 109: LOG(ERROR) << "Unable to flush root index block" << ptr.ToString(); This is not likely to work as intended because a non-OK Status was returned from FinishAndWriteBlock(), resulting in the out-parameter 'ptr' being left in an undefined state (probably untouched). Actually in this case I think we should remove the ERROR log and instead put the same information in the return Status object like: return s.CloneAndPrepend("Unable to flush root index block"); http://gerrit.cloudera.org:8080/#/c/10505/1/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/10505/1/src/kudu/fs/log_block_manager.cc@2041 PS1, Line 2041: VLOG(2) << Substitute("Added block: offset $0, length $1, id $2", nit: for consistency i have the same suggestion for ordering in this file (here and below) as in the other file http://gerrit.cloudera.org:8080/#/c/10505/1/src/kudu/tserver/tablet_copy_source_session.cc File src/kudu/tserver/tablet_copy_source_session.cc: http://gerrit.cloudera.org:8080/#/c/10505/1/src/kudu/tserver/tablet_copy_source_session.cc@395 PS1, Line 395: LOG(DFATAL) << "Data block " << block_id.ToString() << " disappeared: " << s.ToString(); This isn't necessary because we already included it in the error message on the line above -- To view, visit http://gerrit.cloudera.org:8080/10505 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I895da1cc04ecbf006f412f06b31461b03072d32d Gerrit-Change-Number: 10505 Gerrit-PatchSet: 1 Gerrit-Owner: Anupama Gupta <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Thu, 24 May 2018 22:00:31 +0000 Gerrit-HasComments: Yes
