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 11: (3 comments) > And here are another three comments, may i fix them? Go ahead. I agree with your first comment, disagree with the second, and don't care either way on the third. http://gerrit.cloudera.org:8080/#/c/12075/11/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/12075/11/src/kudu/fs/log_block_manager.cc@679 PS11, Line 679: CONTAINER_DISK_FAILURE(block_manager_->env_->SyncDir(data_dir_->dir()), > Still, i think it wastes IO to call 'SyncDir' after file deletion. The rogu Now that we're calling SyncDir for each deleted container instead of just once after Repair(), I agree. http://gerrit.cloudera.org:8080/#/c/12075/11/src/kudu/fs/log_block_manager.cc@2452 PS11, Line 2452: string data_filename = StrCat(container->ToString(), kContainerDataFileSuffix); : uint64_t reported_size; : s = env_->GetFileSizeOnDisk(data_filename, &reported_size); : if (!s.ok()) { : HANDLE_DISK_FAILURE(s, error_manager_->RunErrorNotificationCb( : ErrorHandlerType::DISK_ERROR, dir)); : *result_status = s.CloneAndPrepend(Substitute( : "Could not get on-disk file size of container $0", container->ToString())); : return; : } : int64_t cleanup_threshold_size = container->live_bytes_aligned() * : (1 + FLAGS_log_container_excess_space_before_cleanup_fraction); : if (reported_size > cleanup_threshold_size) { : local_report.full_container_space_check->entries.emplace_back( : container->ToString(), reported_size - container->live_bytes_aligned()); : : // If the container is to be deleted outright, don't bother repunching : // its blocks. The report entry remains, however, so it's clear that : // there was a space discrepancy. : if (container->live_blocks()) { : need_repunching.insert(need_repunching.end(), : dead_blocks.begin(), dead_blocks.end()); : } : } > It's not necessary to clean up threshold size for a dead container, though True, although the FsReport we generate tries to account for all inconsistencies even if they're cleaned up by deleting containers. Plus GetFileSizeOnDisk isn't particularly expensive. http://gerrit.cloudera.org:8080/#/c/12075/11/src/kudu/fs/log_block_manager.cc@2514 PS11, Line 2514: AddNewContainerUnlocked(container); : MakeContainerAvailableUnlocked(std::move(container)); > It's a little bit strange to add the dead containers here and then remove t Yeah but it's simpler in that all containers are treated the same way. If you like, you could change the plumbing and avoid adding dead containers; when I updated your patch it felt a little bit out of scope for me so I didn't do it. -- 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: 11 Gerrit-Owner: helifu <hzhel...@corp.netease.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: helifu <hzhel...@corp.netease.com> Gerrit-Comment-Date: Wed, 09 Jan 2019 05:32:39 +0000 Gerrit-HasComments: Yes
