Hao Hao has posted comments on this change. Change subject: KUDU-1943: Add BlockTransaction to Block Manager ......................................................................
Patch Set 20: (2 comments) http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: Line 132 > Looks like this is being retained as a method on both implementations, but Adar earlier made the point "it shouldn't be part of the WritableBlock interface because there's no need for anyone outside the block manager to know about the distinction." It makes sense to me from a interface point of view. I prefer to keep the change (which removed the interface) unless you have a strong preference? http://gerrit.cloudera.org:8080/#/c/7207/32/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: PS32, Line 1217: : // DoCloseBlocks() has unlocked the container; it may be locked by someone else. : // But block_manager_ is immutable, so this is safe. : return container_->block_manager()->DeleteBlock(id()); : } : : const BlockId& LogWritableBlock::id() const { : return block_id_; : } > Abort() is part of the public API of a WritableBlock, so theoretically, any Makes sense. I don't see a case we would want to Abort() the block if it is successfully closed for now (could be I miss something?). So I prefer to do CHECK on the state. -- 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: 20 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: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
