Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12075 )
Change subject: KUDU-2636: LBM supports deleting dead and full containers ...................................................................... Patch Set 4: (9 comments) http://gerrit.cloudera.org:8080/#/c/12075/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12075/4//COMMIT_MSG@10 PS4, Line 10: presure pressure http://gerrit.cloudera.org:8080/#/c/12075/4//COMMIT_MSG@12 PS4, Line 12: it Nit: "the container" http://gerrit.cloudera.org:8080/#/c/12075/4/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/12075/4/src/kudu/fs/log_block_manager-test.cc@1771 PS4, Line 1771: CheckMetrics({0, 0, 0, 0, 0, 0, 0}); I think you need to wrap calls to CheckMetrics in NO_FATALS also. http://gerrit.cloudera.org:8080/#/c/12075/4/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12075/4/src/kudu/fs/log_block_manager.cc@1237 PS4, Line 1237: // Given the shared ownership of LogBlockDeletionTransaction, at this point : // all registered blocks should be destructed. Thus, 1)coalescing deletions : // for blocks that are contiguous in a container and schedules hole punching; : // 2)schedules containers removal by data dir. How about: // Due to the shared ownership of LogBlockDeletionTransaction, the destructor is // responsible for destroying all registered blocks. This includes: // 1. Punching holes in deleted blocks, and // 2. Deleting dead containers outright. http://gerrit.cloudera.org:8080/#/c/12075/4/src/kudu/fs/log_block_manager.cc@1288 PS4, Line 1288: DataDir* dir = entry.first; : vector<string> names(entry.second.begin(), entry.second.end()); Nit: you can inline this into the Bind() call, the way it was done on L1280. http://gerrit.cloudera.org:8080/#/c/12075/4/src/kudu/fs/log_block_manager.cc@1979 PS4, Line 1979: // Invalidate the cache and remove from the disk. Could you consolidate this with the equivalent code in Repair()? http://gerrit.cloudera.org:8080/#/c/12075/4/src/kudu/fs/log_block_manager.cc@1980 PS4, Line 1980: for (auto& name : names) { 'name' can be a cref. http://gerrit.cloudera.org:8080/#/c/12075/4/src/kudu/fs/log_block_manager.cc@1983 PS4, Line 1983: file_cache_.Invalidate(data_file_name); : file_cache_.Invalidate(metadata_file_name); Why do we need to call Invalidate in this code path? We're not renaming anything. http://gerrit.cloudera.org:8080/#/c/12075/4/src/kudu/fs/log_block_manager.cc@1985 PS4, Line 1985: WARN_NOT_OK_LBM_DISK_FAILURE(env_->DeleteFile(data_file_name), : "Could not delete dead container data file " + data_file_name); : WARN_NOT_OK_LBM_DISK_FAILURE(env_->DeleteFile(metadata_file_name), : "Could not delete dead container metadata file " + metadata_file_name); Shouldn't we delete through the file cache? I.e. file_cache_.DeleteFile? -- To view, visit http://gerrit.cloudera.org:8080/12075 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7d3672ae3c3dd9acd489120d653c44a641537f10 Gerrit-Change-Number: 12075 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: Mon, 17 Dec 2018 19:21:37 +0000 Gerrit-HasComments: Yes
