Wang Xixu 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 59: (10 comments) http://gerrit.cloudera.org:8080/#/c/18569/1/src/kudu/fs/dir_manager.h File src/kudu/fs/dir_manager.h: http://gerrit.cloudera.org:8080/#/c/18569/1/src/kudu/fs/dir_manager.h@46 PS1, Line 46: // Convert a rocksdb::Status to a kudu::Status. RdbStatusToKuduStatus http://gerrit.cloudera.org:8080/#/c/18569/1/src/kudu/fs/dir_manager.cc File src/kudu/fs/dir_manager.cc: http://gerrit.cloudera.org:8080/#/c/18569/1/src/kudu/fs/dir_manager.cc@131 PS1, Line 131: Shutdown(); : } : : Status Dir::OpenRocksDB() { : CHECK_STREQ(FLAGS_block_manager.c_str(), "logr"); : if (db_ != nullptr) { : // Check 'db_' is only possible to be non-nullptr in test environments. : // Some unit tests (e.g. BlockManagerTest.PersistenceTest) will reopen the block manager, : // 'db_' is non-nullptr in this case. : CHECK(!GetTestDataDirectory().empty()) << : Substitute("It's not allowed to reopen the RocksDB $0 except in tests", dir_); : return Status::OK(); : } It is better to read configure from Kudu gflagfile http://gerrit.cloudera.org:8080/#/c/18569/1/src/kudu/fs/dir_manager.cc@172 PS1, Line 172: .prefix_ better to rename it is_rdb_init http://gerrit.cloudera.org:8080/#/c/18569/58/src/kudu/fs/dir_manager.cc File src/kudu/fs/dir_manager.cc: http://gerrit.cloudera.org:8080/#/c/18569/58/src/kudu/fs/dir_manager.cc@146 PS58, Line 146: opts.create_if_missing = true; Could you define a flag for this parameter, and give some comments about the meaning? http://gerrit.cloudera.org:8080/#/c/18569/58/src/kudu/fs/dir_manager.cc@197 PS58, Line 197: dir_ JoinPathSegments(dir_, "rdb")? Maybe define a parameter rdb_path. http://gerrit.cloudera.org:8080/#/c/18569/58/src/kudu/fs/dir_manager.cc@198 PS58, Line 198: delete db_; : db_ = nullptr; How about using unique_ptr for db_? http://gerrit.cloudera.org:8080/#/c/18569/1/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/18569/1/src/kudu/fs/fs_manager.cc@340 PS1, Line 340: } else { Use Enum type to represent file, log, logr http://gerrit.cloudera.org:8080/#/c/18569/58/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: http://gerrit.cloudera.org:8080/#/c/18569/58/src/kudu/fs/log_block_manager.h@423 PS58, Line 423: EstimateContainerCount Please give some comments. http://gerrit.cloudera.org:8080/#/c/18569/58/src/kudu/fs/log_block_manager.h@562 PS58, Line 562: return children_count / 2; Why can not get the correct container number? medata + data file? http://gerrit.cloudera.org:8080/#/c/18569/1/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/18569/1/src/kudu/fs/log_block_manager.cc@1269 PS1, Line 1269: const st The parameter 'id' is not used in this function. The design of this interface is a little strange -- 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: 59 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, 08 Nov 2023 08:32:06 +0000 Gerrit-HasComments: Yes
