Yingchun Lai 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 71: (16 comments) I've reduced some down_cast by introducing a "rocksdb::DB* const rdb_;" member to class LogBlockContainerRdbMeta. http://gerrit.cloudera.org:8080/#/c/18569/61//COMMIT_MSG Commit Message: PS61: > Right, there are a few main themes behind: Thank you for pointing out the issues. 1. Even it change license in new version, we can still use the old version under APLv2. We should keep in mind to check the license when upgrade thirdparty libraries. 2. It is an issue. I will explore in depth to reduce the binaries size in another patches. Maybe we can use its C APIs (https://github.com/facebook/rocksdb/blob/main/include/rocksdb/c.h) 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, : string dir, > nit for here and elsewhere: in case of a method's signature with many param Done http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@216 PS66, Line 216: K_STREQ(FLAGS_block_manager.c_str(), "logr"); > nit: try to rephrase this a bit for a better readility Done http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@217 PS66, Line 217: > nit: reopen Done http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@219 PS66, Line 219: // 'd > nit: does it makes sense to change this to DCHECK, similar to DCHECK_STREQ Done http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@242 PS66, Line 242: // opts.max_write_buffer_number > nit: move this closer to the point of usage Done http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@271 PS66, Line 271: > in-flight Done http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@271 PS66, Line 271: > nit: drop the comma Done http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@271 PS66, Line 271: > there aren't any Done http://gerrit.cloudera.org:8080/#/c/18569/66/src/kudu/fs/dir_manager.cc@278 PS66, Line 278: . > reduces bootstrapping time upon next start-up. Done 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 cor Done 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: } // namespace kudu > Please add a small comment describing the essence of this method's function Done 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: ith the LogBlockManagerNativeMeta, th > Thank you for the explanation! Done http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/log_block_manager.h@657 PS61, Line 657: > Ah, so a 'fatal inconsistency' is when there isn't a way to safely recover/ Done http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/log_block_manager.cc@1987 PS61, Line 1987: .ok() && !s_data.IsNo > It's copied from LogBlockContainerNativeMeta::CheckContainerFiles(). 'static' can not be used here, because there are some parameterized tests (enable encryption or not) re-run these code, and the static variable 'kEncryptionHeaderSize' is immutable, which cause some tests failed. 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: _t* limit = FindFloorOrNull(kPerFsBloc > Maybe, it's possible to introduce a new virtual method into the base class Good idea! Done -- 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: 71 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: Wed, 10 Jan 2024 15:18:31 +0000 Gerrit-HasComments: Yes
