Dan Burkert has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager ......................................................................
Patch Set 13: (19 comments) http://gerrit.cloudera.org:8080/#/c/7207/5//COMMIT_MSG Commit Message: PS5, Line 12: potentionaly typo: potentially http://gerrit.cloudera.org:8080/#/c/7207/13//COMMIT_MSG Commit Message: PS13, Line 10: creates s/creates which happen/new blocks PS13, Line 14: I FinalizeWrite() t add commas (API, FinalizeWrite(), to...) PS13, Line 15: resued reused PS13, Line 15: , no comma here Line 19: https://docs.google.com/a/cloudera.com/document/d/15zZaKeLENRGC_AdjF2N3hxtM_6_OuydXA1reWvDbbK0/edit?usp=sharing I think you may need to make this viewable from anyone (I tried in an incognito window and it asked me to log into my cloudera gmail account). http://gerrit.cloudera.org:8080/#/c/7207/13/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: PS13, Line 74: results s/results/resulting Line 88: // No more data may be written to the block, but it is not yet closed. 'not yet closed' doesn't really distinguish this state from CLOSED. I think this would be more clear as 'but it is not yet guaranteed to be durably stored on disk'. Line 281: BlockTransaction() {} prefer 'BlockTransaction() = default;' Line 283: ~BlockTransaction() { Can't this just be set to the default dtor? ~BlockTransaction() = default; PS13, Line 288: push_back( emplace_back PS13, Line 293: CommitCreation Might be more clear as 'CommitCreatedBlocks'. Line 305: Status s = bm->CloseBlocks(blocks); Would it be difficult to change BlockManager::CloseBlocks to take a 'const vector<unique_ptr<WritableBlock>>&' so you don't have to make a vector copy here? If that would require changing a bunch of other call sites, feel free to ignore. http://gerrit.cloudera.org:8080/#/c/7207/13/src/kudu/fs/file_block_manager.cc File src/kudu/fs/file_block_manager.cc: Line 727: // dirty pages if 'block_manager_flush_on_finalize' is true. I think this should be doing an async flush regardless of the block_manager_flush_on_finalize flag value. Unfortunately I think that means you will need to copy most of the body of Finalize and remove the check (maybe abstract it out?), but I think we definitely want to parallel flushing to happen here. block_coalesce_close shouldn't be tied to the flush on finalize flag. 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, It looks like the only place that calls FinishBlock calls it with an OK status, so I think the s param can be removed. PS13, Line 522: unnecessarily unnecessary Line 1234: Status DoClose(CloseFlags flags); Could DoClose and CloseFlags be private? Line 1911: // dirty pages if 'block_manager_flush_on_finalize' is true. Same thing here - I think we want to do this depending only on the block_coalesce_close flag. PS13, Line 1917: belong s/belong/belonging -- 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: 13 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
