helifu has posted comments on this change. ( http://gerrit.cloudera.org:8080/12239 )
Change subject: [fs]: delete the (orphaned) metadata file when repairing ...................................................................... Patch Set 4: (12 comments) http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc@1896 PS4, Line 1896: const auto CheckRepaired = [&] () { > If you really want to test that the repair happened, you should look at the Done http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc@1987 PS4, Line 1987: // Create a metadata file with 1 byte which is <MIN. : unique_ptr<WritableFile> metadata_file_writer; : ASSERT_OK(env_->NewWritableFile(metadata_file_name, &metadata_file_writer)); : ASSERT_OK(metadata_file_writer->Append(Slice("a"))); : metadata_file_writer->Close(); > Since this pattern is repeated quite a few times, could you encapsulate it Done http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager-test.cc@2029 PS4, Line 2029: // Delete the only block. : vector<BlockId> deleted; : shared_ptr<BlockDeletionTransaction> transaction = bm_->NewDeletionTransaction(); : transaction->AddDeletedBlock(block_id); : ASSERT_OK(transaction->CommitDeletedBlocks(&deleted)); : transaction.reset(); : // Wait for the actual hole punching to take place. : for (const auto& data_dir : dd_manager_->data_dirs()) { : data_dir->WaitOnClosures(); : } > Likewise, encapsulate this in a lambda. Done http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@774 PS1, Line 774: *-------------*------------*----------------*-------------------------*---------------------*/ > I don't think the order of which file is created/deleted matters because wi Agreed:) http://gerrit.cloudera.org:8080/#/c/12239/1/src/kudu/fs/log_block_manager.cc@801 PS1, Line 801: return Status::OK(); > If you look at all the filesystem calls made in this file, they're wrapped Done http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@762 PS4, Line 762: /* Note: the status here only represents the result of check. > Could you reformat this to use multi-line C++ comments? Done http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@767 PS4, Line 767: EXIST && NO LIVE BLCOKS | EXIST && LIVE BLCOKS| > BLOCKS Done http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@769 PS4, Line 769: (new case) > Don't need this; if you really want to distinguish between the old and new Done http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@830 PS4, Line 830: report->incomplete_container_check->entries.emplace_back(common_path); : return Status::Aborted(Substitute("orphaned empty metadata and data files $0", common_path)); > Nit: looks like these lines are indented too much. Done http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@839 PS4, Line 839: if (PREDICT_FALSE(s_data.IsNotFound())) { : if (metadata_size >= pb_util::kPBContainerMinimumValidLength) { > Can combine these and save one level of indentation. Done http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@863 PS4, Line 863: if ((read_status.IsEndOfFile() && live_blocks.empty())) { > Nit: there's an extra set of parens here. Done http://gerrit.cloudera.org:8080/#/c/12239/4/src/kudu/fs/log_block_manager.cc@865 PS4, Line 865: return Status::Aborted(Substitute("orphaned metadata file $0",common_path)); > Nit: need an extra space between the comma and 'common_path' Done -- To view, visit http://gerrit.cloudera.org:8080/12239 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I2a4223d0e6385a7f7776f8fe2679a3bb16d832ab Gerrit-Change-Number: 12239 Gerrit-PatchSet: 4 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Wed, 23 Jan 2019 11:27:53 +0000 Gerrit-HasComments: Yes
