Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/18709 )
Change subject: KUDU-3371 [fs] make LogBlockManager 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 FileBlo Done 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 Done 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 LogBlockM Done http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@12 PS8, Line 12: manage > manages Done http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@15 PS8, Line 15: member functions > the member functions Done http://gerrit.cloudera.org:8080/#/c/18709/8//COMMIT_MSG@15 PS8, Line 15: base > the base Done 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 Yes, I reverted some modifications. 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; Yes it's not needed, but maybe help to serve as documentation. I find the Google C++ code style guide suggest to add override for destructors. > Explicitly annotate overrides of virtual functions or virtual destructors > with exactly one of an override or (less frequently) final specifier. Do not > use virtual when declaring an override. Rationale: A function or destructor > marked override or final that is not an override of a base class virtual > function will not compile, and this helps catch common errors. The specifiers > serve as documentation; if no specifier is present, the reader has to check > all ancestors of the class in question to determine if the function or > destructor is virtual or not. And there is an interesting discussion about it https://github.com/isocpp/CppCoreGuidelines/issues/721, so IMO keep the 'override' maybe help for readers. 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; The Tidy Bot report an error on PS1, so I changed it. > warning: default arguments on virtual or override methods are prohibited > [google-default-arguments] It report it because I'm happen to modify this file. Maybe separate these kind of refactors to another patch would be better. 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 pa That's true, I reverted it, and kept the minimal protected scope. 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' Done 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 It's useful in next patches but now seems useless, as Yuqi also confused about it. Let's add it back in following up patches if it's needed. http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@474 PS8, Line 474: into > in Done http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@474 PS8, Line 474: sequential > sequentially Done http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@474 PS8, Line 474: i.e. > e.g. Done http://gerrit.cloudera.org:8080/#/c/18709/8/src/kudu/fs/log_block_manager.h@475 PS8, Line 475: kSequentialWrittenFile > kSequentiallyWrittenFile Done 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. Improved, but I'm not sure if it is appropriate. 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 Done -- 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 <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Sun, 08 Jan 2023 16:21:30 +0000 Gerrit-HasComments: Yes
