Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/21295 )
Change subject: KUDU-3371 check for RocksDB dir presence upon opening FSManager ...................................................................... Patch Set 5: (13 comments) http://gerrit.cloudera.org:8080/#/c/21295/4/src/kudu/fs/dir_manager.h File src/kudu/fs/dir_manager.h: http://gerrit.cloudera.org:8080/#/c/21295/4/src/kudu/fs/dir_manager.h@155 PS4, Line 155: : const DirInstanceMetadataFile* instance() const { : > Is this still needed? Done http://gerrit.cloudera.org:8080/#/c/21295/4/src/kudu/fs/dir_manager.h@213 PS4, Line 213: rocksdb::DB* rdb(); : : private: : // Initialize the > While you are working in this area of the code, could you move Prepare() me Yes, it can be private now, and I renamed it to InitRocksDBInstance(). Done http://gerrit.cloudera.org:8080/#/c/21295/4/src/kudu/fs/dir_manager.h@224 PS4, Line 224: > nit: opening DirManager Done http://gerrit.cloudera.org:8080/#/c/21295/4/src/kudu/fs/dir_manager.h@474 PS4, Line 474: ::unordered_set<std::string> created_fs_dir_paths_; > nit: Done http://gerrit.cloudera.org:8080/#/c/21295/4/src/kudu/fs/dir_manager.h@474 PS4, Line 474: : : // Directories tracked by this manag > nit: Done http://gerrit.cloudera.org:8080/#/c/21295/4/src/kudu/fs/dir_manager.h@478 PS4, Line 478: > What's the projected number of elements in this container? Is it prudent t The number of elements is the sum of --wal_dir and --data_dirs, less than 100 in regular deployments. It's OK to use std::unordered_set instead. http://gerrit.cloudera.org:8080/#/c/21295/4/src/kudu/fs/dir_manager.h@478 PS4, Line 478: > nit: to avoid confusion with the 'dirs_' field below, consider renaming thi Sure. http://gerrit.cloudera.org:8080/#/c/21295/4/src/kudu/fs/dir_manager.cc File src/kudu/fs/dir_manager.cc: http://gerrit.cloudera.org:8080/#/c/21295/4/src/kudu/fs/dir_manager.cc@215 PS4, Line 215: !metadata_file_->healthy()) { > Instead of saving this boolean flag into a member field, maybe pass it as a Done http://gerrit.cloudera.org:8080/#/c/21295/4/src/kudu/fs/dir_manager.cc@217 PS4, Line 217: > nit: opening Done http://gerrit.cloudera.org:8080/#/c/21295/4/src/kudu/fs/dir_manager.cc@218 PS4, Line 218: > here and below: why to use this instance() accessor when 'metadata_file_' i Done http://gerrit.cloudera.org:8080/#/c/21295/4/src/kudu/fs/dir_manager.cc@223 PS4, Line 223: // Note: the unhealthy directories will be kept, but will be skipped when opening block manager. > Does it make sense to log a message (INFO?) for the 'else' part when instan I added a WARNING message and short return when it's not healthy. http://gerrit.cloudera.org:8080/#/c/21295/4/src/kudu/fs/dir_manager.cc@484 PS4, Line 484: } > nit: could this be Done http://gerrit.cloudera.org:8080/#/c/21295/4/src/kudu/fs/fs_manager-test.cc File src/kudu/fs/fs_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/21295/4/src/kudu/fs/fs_manager-test.cc@1533 PS4, Line 1533: pts.create_if_missing = true; > Does it make sense to add the 'error_if_exists' option as well to mimic wha Done -- To view, visit http://gerrit.cloudera.org:8080/21295 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iab4bffc6b902ab96edf0ca6c44f51c8db2670d52 Gerrit-Change-Number: 21295 Gerrit-PatchSet: 5 Gerrit-Owner: Yingchun Lai <laiyingc...@apache.org> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org> Gerrit-Comment-Date: Tue, 11 Jun 2024 15:57:36 +0000 Gerrit-HasComments: Yes
