Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19610 )
Change subject: KUDU-3371 [fs] make LogBlockContainer a base class ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/19610/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19610/1//COMMIT_MSG@12 PS1, Line 12: the a http://gerrit.cloudera.org:8080/#/c/19610/1//COMMIT_MSG@13 PS1, Line 13: the sequential written file a sequentially written file http://gerrit.cloudera.org:8080/#/c/19610/1//COMMIT_MSG@14 PS1, Line 14: it is how we do as before that's what LogBlockContainer used to be prior to this change http://gerrit.cloudera.org:8080/#/c/19610/1//COMMIT_MSG@16 PS1, Line 16: Most of the member functions are left in the base class : LogBlockContainer, I'm planning to add a new derived class which : stores containers' metadata into RocksDB in next patches to avoid It's OK to have a bigger change that would make this part at least logically and structurally sound. IIUC, right now there are many methods in the 'base' LogBlockContainer class that don't look relevant to operations to be run on metadata stored in RocksDB. The most important point of separating these patches is to have them logically self-contained and sound from the design perspective. The size of a patch is rather a secondary concern, IMO. Would you consider revving this patch to make LogBlockContainer closer to the end result, but still have it separate from the rest of the RocksDB-related updates? In other words, consider making LogBlockContainer a real base class or even an interface with so that derived classes would work with log block manager and RocksDB, correspondingly? http://gerrit.cloudera.org:8080/#/c/19610/1//COMMIT_MSG@19 PS1, Line 19: updating updates http://gerrit.cloudera.org:8080/#/c/19610/1/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/19610/1/src/kudu/fs/log_block_manager.cc@210 PS1, Line 210: using internal::LogBlock; : using internal::LogBlockContainer; : using internal::LogBlockContainerNativeMeta; : using internal::LogBlockDeletionTransaction; : using internal::LogWritableBlock; : using pb_util::ReadablePBContainerFile; : using pb_util::WritablePBContainerFile; : using std::accumulate; : using std::map; : using std::optional; : using std::set; : using std::shared_ptr; : using std::string; : using std::unique_ptr; : using std::unordered_map; : using std::unordered_set; : using std::vector; : using strings::Substitute; nit: move these declarations out of the namespace -- 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: 1 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: 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: Wed, 22 Mar 2023 23:12:48 +0000 Gerrit-HasComments: Yes
