Dan Burkert has posted comments on this change. Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager ......................................................................
Patch Set 3: (16 comments) http://gerrit.cloudera.org:8080/#/c/7207/3//COMMIT_MSG Commit Message: PS3, Line 10: happen in a : logic operation 'which happen in a logical operation' Line 20: Add a link to the design doc. http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/cfile/bloomfile.h File src/kudu/cfile/bloomfile.h: Line 47: // Close the bloom's CFile, releasing the underlying block to 'closer'. Update this comment. http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/cfile/cfile_writer.h File src/kudu/cfile/cfile_writer.h: Line 109: // Close the CFile and release the underlying writable block to 'closer'. Update this comment. http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: Line 67: // Close() is an expensive operation, as it must flush both dirty block data This comment should be updated to explain Finalize. Line 142: // Finalize a fully written block. Add more info about what 'finalize' means in this context, either here or in the class doc. Line 143: virtual Status FinalizeWrite() = 0; Perhaps we could simplify this interface by combining 'FlushDataAsync' and 'FinalizeWrite'. Put another way - do we always want to do these operations together, or are there cases where one is done without the other? Line 144: On the naming of 'FinalizeWrite' - the name is good, but I'd be in favor of shortening to 'Finalize'. I think the 'Write' part is understood given the context of a WritableBlock. Line 283: // Blocks must be closed explicitly via CloseBlocks(), otherwise they will Update comment Line 305: Status CommitWrites() { Hmm I know we discussed on slack (or the design doc?) why CommitWrites needs to be separate of CommitDeletes, but I've lost that context now. Might be good to explain why there split as part of the method comments. http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: Line 429: bm_->all_containers_by_name_.size(); Is this needed? Line 915: BlockTransaction transaction; Should this be committing the writes? Seems strange the previous version didn't either. Line 1107: BlockTransaction transaction; Same here. http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: Line 274: Status FinishBlocks(std::set<WritableBlock*> blocks); Any reason to prefer a set here to a vector? Vectors are typically lighter weight. Line 367: // block is fully written. 'should_update' indicates if it should round up Can't this method internally determine whether the round up has already been done based on whether the block's state is DIRTY or FINALIZED? http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/tablet/diskrowset.h File src/kudu/tablet/diskrowset.h: Line 90: // Closes the CFiles, releasing the underlying blocks to 'closer'. Update comment. -- 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: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao <[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
