Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18569 )
Change subject: KUDU-3371 [fs] Use RocksDB to store LBM metadata ...................................................................... Patch Set 62: (16 comments) Thank you for revving the patch! Overall looks better than a few patchsets ago, but I'm too many down_casts bother me. I'll take a closer look, trying to suggest a way to get rid of those. Meanwhile, maybe you could also take a look at possible options to minimize using down_cast? Using down_cast in many places is a sign that the class hierarchy needs to be revisited. http://gerrit.cloudera.org:8080/#/c/18569/61//COMMIT_MSG Commit Message: PS61: > Yep, you ever asked about it [1]. Right, there are a few main themes behind: 1. Protect against possible licensing issues (if they change the license of a whim) 2. Reduce the size of RELEASE Kudu binaries. If taking a brief view on that, it's not clear how to achieve that sort of behavior for RocksDB library. Since RocksDB's API/ABI is C++, not C, dlopen() isn't going to work as easy as it would do for C API/ABI. I was just curious whether you've done any further research in that direction. http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc File src/kudu/fs/dir_manager.cc: http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@208 PS66, Line 208: RdbDir::RdbDir(Env* env, DirMetrics* metrics, FsType fs_type, std::string dir, : std::unique_ptr<DirInstanceMetadataFile> metadata_file, : std::unique_ptr<ThreadPool> pool) nit for here and elsewhere: in case of a method's signature with many parameters spread across multiple lines, consider placing each parameter in its own line -- that improves readability and makes it easier to maintain and track changes in the long run http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@216 PS66, Line 216: Check 'db_' is only possible to be valid in test environments when OpenRocksDB(). nit: try to rephrase this a bit for a better readility http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@217 PS66, Line 217: will reopen nit: reopen It's better to use the present tense when describing facts that are true in the current version of the code. http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@219 PS66, Line 219: CHECK nit: does it makes sense to change this to DCHECK, similar to DCHECK_STREQ above (since the code paths that could call this method are static, a misuse/mistake would be caught in DEBUG builds anyway)? http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@242 PS66, Line 242: rocksdb::BlockBasedTableOptions tbl_opts; nit: move this closer to the point of usage http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@268 PS66, Line 268: return; nit: is it expected to call Shutdown() multiple times, or that would be a programming mistake? If the latter, consider adding corresponding DCHECK() here. http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@271 PS66, Line 271: , nit: drop the comma http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@271 PS66, Line 271: there is no there aren't any http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@271 PS66, Line 271: in flight in-flight http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@278 PS66, Line 278: ich cause longer time to shut down server. reduces bootstrapping time upon next start-up. http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@294 PS66, Line 294: } Is it OK to call this even when 'db_' is null? If not, consider adding corresponding DCHECK() to catch programming mistakes. http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/log_block_manager.h@671 PS66, Line 671: }; Please add a small comment describing the essence of this method's functionality, and document the parameters as well (similar to the doc for the DeleteContainerMetadata method above). http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/log_block_manager.h@599 PS61, Line 599: The data in RocksDB will be compacted > When the tserver shut down normally, the RocksDB object will be destoryed i Thank you for the explanation! http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/log_block_manager.h@657 PS61, Line 657: > In fact, I copied this comment from the super class, I will leave the "fata Ah, so a 'fatal inconsistency' is when there isn't a way to safely recover/ignore the error and continue with the rest of server's activity without a risk of data corruption. Thank you for the clarification. http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/log_block_manager.cc@3979 PS66, Line 3979: lockContainerRdbMeta::Open(this, dir, Maybe, it's possible to introduce a new virtual method into the base class called something like Prepare(), and implement it as no-op for Dir::Prepare(), while do all the necessary operations there for RdbDir::Prepare()? -- To view, visit http://gerrit.cloudera.org:8080/18569 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie72f6914eb5653a9c034766c6cd3741a8340711f Gerrit-Change-Number: 18569 Gerrit-PatchSet: 62 Gerrit-Owner: Yingchun Lai <laiyingc...@apache.org> Gerrit-Reviewer: Abhishek Chennaka <achenn...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: KeDeng <kdeng...@gmail.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yifan Zhang <chinazhangyi...@163.com> Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org> Gerrit-Reviewer: Yuqi Du <shenxingwuy...@gmail.com> Gerrit-Comment-Date: Sat, 06 Jan 2024 04:41:51 +0000 Gerrit-HasComments: Yes
