Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19610 )
Change subject: KUDU-3371 [fs] make LogBlockContainer a base class ...................................................................... Patch Set 3: (2 comments) Thanks a lot to your guys to review this patch! http://gerrit.cloudera.org:8080/#/c/19610/3/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/19610/3/src/kudu/fs/log_block_manager.cc@629 PS3, Line 629: virtual void AfterBlocksDeleted() {} > nit: what about How about 'PostWorkAfterBlocksDeleted()'? Not mind to change it, but is there any reason to define it as pure virtual function just because it's a new function? http://gerrit.cloudera.org:8080/#/c/19610/3/src/kudu/fs/log_block_manager.cc@743 PS3, Line 743: LogBlockContainerNativeMeta > In semantics, LogBlockContainer is a pair combination of a meta file and a It is corresponding to LogBlockManagerNativeMeta in https://github.com/apache/kudu/blob/master/src/kudu/fs/log_block_manager.h#L492 If introducing a new class LogBlockMeta, and it compose LogBlockContainer with the data part logic, I think there are two ways to use the LogBlockContainer: 1. Expose LogBlockMeta as accessable variable or provide a public getter. When operate on the container, you should distinguish data or metadata part you want to operate on, it will break the encapsulation of the container. 2. Hide LogBlockMeta, provide duplicate functions of LogBlockMeta for LogBlockContainer, the code would be duplicate. Could you please point out what's the outstanding advantages? -- To view, visit http://gerrit.cloudera.org:8080/19610 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieb199de95973aaeba76947ad095907272d84ca67 Gerrit-Change-Number: 19610 Gerrit-PatchSet: 3 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Tue, 04 Apr 2023 15:13:28 +0000 Gerrit-HasComments: Yes
