Andrew Wong has posted comments on this change. Change subject: WIP KUDU-1943: Add BlockTransaction to Block Manager ......................................................................
Patch Set 8: (9 comments) http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: PS8, Line 289: BlockTransaction Besides the name, what's the difference between ScopedWritableBlockCloser and BlockTransaction? Is there a different expected usage now? If so, maybe document it somewhere. PS8, Line 305: Status CommitWrites Can you add docs to this and AddCreatedBlock? Their names aren't as straightforward as AddBlock and CloseBlocks. Also would be worth noting any new behavior. http://gerrit.cloudera.org:8080/#/c/7207/3/src/kudu/fs/file_block_manager.cc File src/kudu/fs/file_block_manager.cc: PS3, Line 237: IDE; nit: use lower-case http://gerrit.cloudera.org:8080/#/c/7207/8/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS8, Line 270: have been finalized What happens if this is called and the blocks are not finalized? PS8, Line 365: 'should_update' indicates if it should round up I think we try to avoid boolean parameters with enums (e.g. a AlignmentAdjustmentType or something with ROUND_UP and NO_ROUNDING). PS8, Line 810: ner file " << data_file_->filename(); This is syncing the data file and metadata file, right? may need another VLOG above the SyncMetadata() call PS8, Line 812: ata()); : RETURN_NOT_OK(SyncMet Why do we need this here and not in FinishBlock? PS8, Line 1174: true maybe enum here too? PS8, Line 1264: VLOG(3) << "Finalizing block " << id(); Should we be logging even if it's CLEAN? -- 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: 8 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
