Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/8144 )
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction ...................................................................... Patch Set 10: (2 comments) http://gerrit.cloudera.org:8080/#/c/8144/10/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: http://gerrit.cloudera.org:8080/#/c/8144/10/src/kudu/fs/block_manager.h@289 PS10, Line 289: // Add a block to the creation transaction. This method is not thread-safe. > I don't think it's appropriate for threads to share BlockCreationTransactio Yeah, makes sense. I was in between to document it in the method level or class level. :) Will update. http://gerrit.cloudera.org:8080/#/c/8144/8/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/8144/8/src/kudu/fs/log_block_manager.cc@1152 PS8, Line 1152: std::unique_ptr<WritableBlock> block) { > Well, both pieces of state is definitely worse than just one, so I'd rather The issue with the third is DoCloseBlocks() takes 'const vector<LogWritableBlock*>&' as parameter and created_block_map stores 'vector<std::unique_ptr<LogWritableBlock>>'. So that means we need to either do the transformation or change the input parameter AFAICT. I don't think doing the later is a good idea, since we are not changing ownership in DoCloseBlocks() and DoCloseBlocks() is called in other places as well. I agree with you that the first and the second changes doesn't seem to be big deals. But adding all of these together, may not better that keeping it as the way it was? Honestly speaking, I personally prefer to revert to the old approach. -- To view, visit http://gerrit.cloudera.org:8080/8144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I60c7d437061f98ad27b9aecda5fa89e909fb2ec6 Gerrit-Change-Number: 8144 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Tue, 10 Oct 2017 23:41:29 +0000 Gerrit-HasComments: Yes
