Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18709 )
Change subject: KUDU-3371 [fs] make LogBlockManager as a base class ...................................................................... Patch Set 8: (18 comments) http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG Commit Message: PS8: FileMetaLogBlockManager looks confusing because of already existing FileBlockManager I'd suggest naming the new class LogBlockManagerNativeMeta or something like that. http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@7 PS8, Line 7: make LogBlockManager as a base class make LogBlockManager a base class http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@9 PS8, Line 9: This patch makes LogBlockManager as a base class, and adds : FileMetaLogBlockManager inherit from it This patch makes LogBlockManager a base class for the newly added LogBlockManagerNativeMeta. http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@12 PS8, Line 12: manage manages http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@15 PS8, Line 15: member functions the member functions http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@15 PS8, Line 15: base the base http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@16 PS8, Line 16: to avoid too large updating on existing code, I'm planning to do : that in next patches. I thought it would be easier to separate those private/protected methods as per their current usage in the new hierarchy in this patch, no? http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@178 PS8, Line 178: override; I guess 'override' or 'virtual' here isn't needed: the base BlockManager class has already introduced the virtual base since its destructor defined as virtual virtual ~BlockManager() {} http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@180 PS8, Line 180: Status Open(FsReport* report, std::atomic<int>* containers_processed, : std::atomic<int>* containers_total) override; Once the signature has changed, it's no longer an override for BlockManager::Open(). This method now shadows BlockManager::Open(), but it doesn't override it. Is that intentional? If yes, then that's not a good practice anyways. BTW, what's wrong with the original signature of the method? Why to change it? http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@200 PS8, Line 200: FRIEND_TEST(LogBlockManagerTest, TestAbortBlock); : FRIEND_TEST(LogBlockManagerTest, TestCloseFinalizedBlock); : FRIEND_TEST(LogBlockManagerTest, TestCompactFullContainerMetadataAtStartup); : FRIEND_TEST(LogBlockManagerTest, TestFinalizeBlock); : FRIEND_TEST(LogBlockManagerTest, TestLIFOContainerSelection); : FRIEND_TEST(LogBlockManagerTest, TestLookupBlockLimit); : FRIEND_TEST(LogBlockManagerTest, TestMetadataTruncation); : FRIEND_TEST(LogBlockManagerTest, TestMisalignedBlocksFuzz); : FRIEND_TEST(LogBlockManagerTest, TestParseKernelRelease); : FRIEND_TEST(LogBlockManagerTest, TestBumpBlockIds); : FRIEND_TEST(LogBlockManagerTest, TestReuseBlockIds); : FRIEND_TEST(LogBlockManagerTest, TestFailMultipleTransactionsPerContainer); : : friend class internal::LogBlockContainer; : friend class internal::LogBlockDeletionTransaction; : friend class internal::LogWritableBlock; : friend class LogBlockManagerTest; I don't think this need to be in the 'protected' part, can be in private part easily. That's also true for many other fields and methods that were private in prior version even if FileMetaLogBlockManager has appeared. http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@472 PS8, Line 472: Where the metadata to store 'Where to store the metadata' or 'Where the metadata is stored' http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@473 PS8, Line 473: enum LogBlockManagerMetadataType { Why to introduce this enumeration at all? Isn't it enough to have a set of virtual methods in the interface to deal with the specifics of the metadata storage specifics? http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@474 PS8, Line 474: sequential sequentially http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@474 PS8, Line 474: into in http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@474 PS8, Line 474: i.e. e.g. http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@475 PS8, Line 475: kSequentialWrittenFile kSequentiallyWrittenFile http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@487 PS8, Line 487: // The metadata part of a container is written sequentially to a metadata file. Please add a more generic comment to express the essence of this class. http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@489 PS8, Line 489: even after a great many even after many -- To view, visit http://gerrit.cloudera.org:8080/18709 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I59c1287a9539668e0c08036452bb96a3006ed356 Gerrit-Change-Number: 18709 Gerrit-PatchSet: 8 Gerrit-Owner: Yingchun Lai <acelyc1112...@gmail.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: KeDeng <kdeng...@gmail.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yifan Zhang <chinazhangyi...@163.com> Gerrit-Reviewer: Yingchun Lai <acelyc1112...@gmail.com> Gerrit-Reviewer: Yuqi Du <shenxingwuy...@gmail.com> Gerrit-Comment-Date: Thu, 05 Jan 2023 03:12:36 +0000 Gerrit-HasComments: Yes
