helifu 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: (6 comments) 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: done. 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 done. 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()? i have no idea :( 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. done. 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 any The function 'FileCache<FileType>::DeleteFile' does not delete file directly, because there is a case to mark the 'flags_' of class BaseDescriptor to be deleted only. Then the 'descriptor_expiry_thread_' of class FileCache erases the descriptor and the file is deleted in the destructor of class BaseDescriptor at last. The default of 'file_cache_expiry_period_ms' is 60 seconds. Thus the 'env_->SyncDir()' later will be meaningless. So, i think it is better to use 'file_cache_.Invalidate'+'env_->DeleteFile'. What do you think? 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? the answer is listed above. -- 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: Tue, 18 Dec 2018 01:27:01 +0000 Gerrit-HasComments: Yes
