Hao Hao has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager ......................................................................
Patch Set 14: (17 comments) http://gerrit.cloudera.org:8080/#/c/7207/13//COMMIT_MSG Commit Message: PS13, Line 10: new blo > s/creates which happen/new blocks Done PS13, Line 15: > no comma here Done PS13, Line 15: reused > reused Done 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 incog Done http://gerrit.cloudera.org:8080/#/c/7207/13/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: PS13, Line 74: resulti > s/results/resulting Done Line 88: // No more data may be written to the block, but it is not yet guaranteed > 'not yet closed' doesn't really distinguish this state from CLOSED. I thin Done Line 281: // Group a set of block writes together in a transaction. This has two > prefer 'BlockTransaction() = default;' Done Line 283: // synchronization for a batch of blocks if possible to achieve better > Can't this just be set to the default dtor? Done PS13, Line 288: > emplace_back Done PS13, Line 293: > Might be more clear as 'CommitCreatedBlocks'. Done Line 305: Status s = bm->CloseBlocks(created_blocks_); > Would it be difficult to change BlockManager::CloseBlocks to take a 'const Updated it as you suggested. 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: > I think this should be doing an async flush regardless of the block_manager Done 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 sta Sorry, I do not quite follow your comment. You mean FinishBlock is always called with 's' as OK status? It does not seem to be the case, either Sync() or AppendMetadata() can return bad status, right? PS13, Line 522: unnecessary I > unnecessary Done Line 1234: // Actually close the block, possibly synchronizing its dirty data and > Could DoClose and CloseFlags be private? Hmm, they are used in other class such as LogBlockManager. Line 1911: record.set_timestamp_us(GetCurrentTimeMicros()); > Same thing here - I think we want to do this depending only on the block_co Done PS13, Line 1917: that > s/belong/belonging Done -- 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
