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 6: (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 When working on this patch, honestly speaking I do not have the intention to completely remove CreateBlock() and DeleteBlock(). But giving more thought doing so may simplify BM to some degree. If that is also your thought, I will update the comment accordingly. 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: atus BloomFileWr > We having "using fs::..." stuff above; you can do the same for BlockManager Done 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: ASSERT_OK(w.Start()); > We having "using fs::..." stuff above; you can do the same for BlockManager Done 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: TRACE_EVENT0("cfile", "CFileWriter::Finish"); > We having "using fs::..." stuff above; you can do the same for BlockManager Done 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: t by this block man > Nit: how about NewCreationTransaction()? And NewDeletionTransaction? Done 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: // On success, guarantees that outstanding data is durable. > I think this and BlockDeletionTransaction should be pure interfaces. It may Makes sense, previously my concern was about having duplicate code, but I agree with the cons of not doing so. http://gerrit.cloudera.org:8080/#/c/8144/4/src/kudu/fs/block_manager.h@325 PS4, Line 325: > The rule of three explanation makes sense, but it might be nice to define a Sure, will update. 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: RETURN_NOT_OK_HANDLE_DISK_FAILURE((status_expr), \ : error_manager_->RunErrorNotificationCb(dd_manager_->FindDataDirByUuidIndex( \ : internal::FileBlockLocatio > Nit: combine Done 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: : // Deletion is forbidden for read-only container. : scoped_refptr<LogBlock> lb; > Nit: combine Done 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: Status DeltaFileWriter::Finish() { > We having "using fs::..." stuff above; you can do the same for BlockManager Done 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: TRACE_EVENT0("tablet", "DiskRowSetWriter::Finish"); > We having "using fs::..." stuff above; you can do the same for BlockManager Done 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: BlockManager* bm = fs_->block_manager(); > We having "using fs::..." stuff above; you can do the same for BlockManager Done 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: tablet_copy_metrics_(tablet_copy_metrics) { > We having "using fs::..." stuff above; you can do the same for BlockManager Done -- 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: 6 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: Thu, 05 Oct 2017 17:25:08 +0000 Gerrit-HasComments: Yes
