Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8144 )
Change subject: KUDU-2055 [part 3]: Refactor BlockCreationTransaction and BlockDeletionTransaction ...................................................................... Patch Set 5: (13 comments) http://gerrit.cloudera.org:8080/#/c/8144/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8144/4//COMMIT_MSG@10 PS4, Line 10: so that in : future they can be extended for specific implementations. Is one of the motivations also to force block creation and deletion to take place via these transactions rather than today's CreateBlock() and DeleteBlock()? If so, it would be good to understand how we'll get there, since that requires some additional refactoring. http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/bloomfile.cc File src/kudu/cfile/bloomfile.cc: http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/bloomfile.cc@116 PS4, Line 116: fs::BlockManager We having "using fs::..." stuff above; you can do the same for BlockManager. http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/cfile-test.cc File src/kudu/cfile/cfile-test.cc: http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/cfile-test.cc@937 PS4, Line 937: fs::BlockManager* bm = fs_manager_->block_manager(); We having "using fs::..." stuff above; you can do the same for BlockManager. http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/cfile_writer.cc File src/kudu/cfile/cfile_writer.cc: http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/cfile/cfile_writer.cc@206 PS4, Line 206: fs::BlockManager* bm = block_->block_manager(); We having "using fs::..." stuff above; you can do the same for BlockManager. http://gerrit.cloudera.org:8080/#/c/8144/5/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: http://gerrit.cloudera.org:8080/#/c/8144/5/src/kudu/fs/block_manager.h@258 PS5, Line 258: CreationTransaction Nit: how about NewCreationTransaction()? And NewDeletionTransaction? http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/block_manager.h@283 PS4, Line 283: class BlockCreationTransaction { I think this and BlockDeletionTransaction should be pure interfaces. It may result in some duplicated code (not much, I think), but I believe it'll let you eliminate BlockManager::CloseBlocks, which AFAICT only exists because BlockCreationTransaction isn't a member of LogBlockManager. The other problem with the transaction classes being concrete classes is that there's nothing stopping me from declaring and using them directly, when the intent of this patch is (I believe) to force people to use CreationTransaction() and DeletionTransaction(). Is this going to be addressed in a follow-on patch? http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/block_manager.h@325 PS4, Line 325: explicit BlockDeletionTransaction(BlockManager* bm) > I read it at: http://en.cppreference.com/w/cpp/language/rule_of_three that The rule of three explanation makes sense, but it might be nice to define a virtual dtor just so this is more consistent with BlockCreationTransaction. It's easy to look at the two classes, see that one doesn't define a virtual dtor, and assume it's a mistake. http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/file_block_manager.cc File src/kudu/fs/file_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/file_block_manager.cc@801 PS4, Line 801: unique_ptr<internal::FileBlockCreationTransaction> transaction( : new internal::FileBlockCreationTransaction(this)); : return std::move(transaction); Nit: combine http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/log_block_manager.cc@1832 PS4, Line 1832: unique_ptr<internal::LogBlockCreationTransaction> transaction( : new internal::LogBlockCreationTransaction(this)); : return std::move(transaction); Nit: combine http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/deltafile.cc File src/kudu/tablet/deltafile.cc: http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/deltafile.cc@116 PS4, Line 116: fs::BlockManager* bm = writer_->block()->block_manager(); We having "using fs::..." stuff above; you can do the same for BlockManager. http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/diskrowset.cc File src/kudu/tablet/diskrowset.cc: http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/diskrowset.cc@231 PS4, Line 231: fs::BlockManager* bm = rowset_metadata_->fs_manager()->block_manager(); We having "using fs::..." stuff above; you can do the same for BlockManager. http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/multi_column_writer.cc File src/kudu/tablet/multi_column_writer.cc: http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tablet/multi_column_writer.cc@123 PS4, Line 123: unique_ptr<BlockCreationTransaction> transaction = bm->CreationTransaction(); We having "using fs::..." stuff above; you can do the same for BlockManager. http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tserver/tablet_copy_client.cc File src/kudu/tserver/tablet_copy_client.cc: http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/tserver/tablet_copy_client.cc@160 PS4, Line 160: fs::BlockManager* bm = fs_manager->block_manager(); We having "using fs::..." stuff above; you can do the same for BlockManager. -- 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: 5 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: Mon, 02 Oct 2017 21:50:33 +0000 Gerrit-HasComments: Yes
