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 11: (3 comments) Thank you very much :) And here are another three comments, may i fix them? 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 rogue admin should be responsible for his operations. Besides, for the kudu newbee, it will bring barriers while rebooting the kudu cluster. Anyway, that's only my opinion. 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 the number of the dead containers is very limited(we know that the dead containers will be deleted automatically at the runtime). 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 them in the 'Repair()'. Maybe we could do something at Line2386. -- 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 03:22:22 +0000 Gerrit-HasComments: Yes
