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 60: (12 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: > RdbStatusToKuduStatus The return type indicate the target type, and the kudu::Status is the deault "Status" we use in Kudu, so I think it would be convenient and obvious to omit the KuduStatus. 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: } : : Status Dir::OpenRocksDB() { : CHECK_STREQ(FLAGS_block_manager.c_str(), "logr"); : if (db_) { : // Check 'db_' is only possible to be valid in test environments when OpenRocksDB(). : // Some unit tests (e.g. BlockManagerTest.PersistenceTest) will reopen the block manager, : // 'db_' is valid in these cases. : 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 Of course, we can improve this in following patches, as the comments in the latest patches mentioned. http://gerrit.cloudera.org:8080/#/c/18569/1/src/kudu/fs/dir_manager.cc@172 PS1, Line 172: .table_f > better to rename it is_rdb_init It has been updated, see the latest patch please. 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: rocksdb::Options opts; > Could you define a flag for this parameter, and give some comments about th Comments have been added, but gflag is not necessary currently, it is always true. http://gerrit.cloudera.org:8080/#/c/18569/58/src/kudu/fs/dir_manager.cc@197 PS58, Line 197: > JoinPathSegments(dir_, "rdb")? Done http://gerrit.cloudera.org:8080/#/c/18569/58/src/kudu/fs/dir_manager.cc@198 PS58, Line 198: : if (db_) { > How about using unique_ptr for db_? Done 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 There are much code assign the string flag FLAGS_block_manager to this option, it will introduce many changes. I think it's not necessary to this patch, we can do this refactor in following patch if you think it's meaningful. 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: count of containers a > Please give some comments. Done http://gerrit.cloudera.org:8080/#/c/18569/58/src/kudu/fs/log_block_manager.h@562 PS58, Line 562: size_t EstimateContainerCoun > Why can not get the correct container number? The data direcory is a common dirctory on the filesystem, Kudu users are possible to place any kind of files there. For example, a backup file will be generated if using 'kudu pbc edit' against a metadata 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 interfa Yes, it has been removed already in the latest patch set. http://gerrit.cloudera.org:8080/#/c/18569/59/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/18569/59/src/kudu/fs/log_block_manager.cc@1899 PS59, Line 1899: WARN_NOT_OK > Should it return when delete data file failed? Would it affects line 1902? Similar to line 1165, it doesn't matter, it is an operation in a do..while loop, another container name is generated in the next iteration. http://gerrit.cloudera.org:8080/#/c/18569/59/src/kudu/fs/log_block_manager.cc@3966 PS59, Line 3966: Initialize data direc > nit: Open rocksDB failed 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: 60 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: Mon, 13 Nov 2023 00:39:47 +0000 Gerrit-HasComments: Yes
