Dan Burkert has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager ......................................................................
Patch Set 14: (4 comments) http://gerrit.cloudera.org:8080/#/c/7207/14/src/kudu/fs/block_manager-stress-test.cc File src/kudu/fs/block_manager-stress-test.cc: Line 282: dirty_blocks.push_back(std::move(block)); use emplace_back with r-value references. http://gerrit.cloudera.org:8080/#/c/7207/14/src/kudu/fs/file_block_manager.cc File src/kudu/fs/file_block_manager.cc: Line 396: Status FileWritableBlock::FlushAndFinalize(FlushMode mode) { Hmm I'm not sure this is doing what we want when block_manager_flush_on_finalize = false. Consider this sequence of events: FLAGS_block_manager_flush_on_finalize = false; FLAGS_block_coalesce_close = true; block.Finalize(); // Finalizes the block, but doesn't flush it block_manager.CloseBlocks( { move(block) } ); // Doesn't flush the block, since it's already finalized. I think in that case CloseBlocks actually should async flush the block. http://gerrit.cloudera.org:8080/#/c/7207/13/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 278: Status FinishBlock(const Status& s, WritableBlock* block, > Sorry, I do not quite follow your comment. You mean FinishBlock is always c Yah I didn't realize it was being called in a closure. Still, you could remove the Status param and change line 1431 to be: s = s.AndThen([&] { return container_->FinishBlock(this, round_mode, block_offset_); }); http://gerrit.cloudera.org:8080/#/c/7207/14/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 1524: RETURN_NOT_OK_PREPEND(s, "Unable to append block metadata"); here and abve, start the message with a lowercase ('unable', 'container'). -- To view, visit http://gerrit.cloudera.org:8080/7207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I01f3ed830cc1f86d7bb8a66b2f780914a88884bd Gerrit-PatchSet: 14 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
