Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21295 )
Change subject: KUDU-3371 Check whether the RocksDB dir incorrectly exist or missing ...................................................................... Patch Set 1: (11 comments) http://gerrit.cloudera.org:8080/#/c/21295/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/21295/1//COMMIT_MSG@9 PS1, Line 9: stroe store ? http://gerrit.cloudera.org:8080/#/c/21295/1//COMMIT_MSG@12 PS1, Line 12: initial initially http://gerrit.cloudera.org:8080/#/c/21295/1//COMMIT_MSG@12 PS1, Line 12: dir directory http://gerrit.cloudera.org:8080/#/c/21295/1/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: http://gerrit.cloudera.org:8080/#/c/21295/1/src/kudu/fs/data_dirs.cc@212 PS1, Line 212: // Initialize the directory only if it's healthy. : if (new_dir->instance()->healthy()) { What if it's not healthy? Where is that handled then? http://gerrit.cloudera.org:8080/#/c/21295/1/src/kudu/fs/dir_manager.h File src/kudu/fs/dir_manager.h: http://gerrit.cloudera.org:8080/#/c/21295/1/src/kudu/fs/dir_manager.h@70 PS1, Line 70: static const std::string kRocksDBDirName = "rdb"; : static const std::string kDefaultTenantName = "default_tenant_kudu"; : static const std::string kDefaultTenantID = "00000000000000000000000000000000"; nit: please re-order these alphabetically based on the name of the constant http://gerrit.cloudera.org:8080/#/c/21295/1/src/kudu/fs/dir_manager.h@155 PS1, Line 155: DirInstanceMetadataFile* instance() const { Make this const-correct: if removing the 'const' from the returned value, remove the 'const' from the method as well if the method now exposes a part of the class's via a non-const pointer. http://gerrit.cloudera.org:8080/#/c/21295/1/src/kudu/fs/dir_manager.h@220 PS1, Line 220: the an http://gerrit.cloudera.org:8080/#/c/21295/1/src/kudu/fs/dir_manager.h@221 PS1, Line 221: existing an existing http://gerrit.cloudera.org:8080/#/c/21295/1/src/kudu/fs/dir_manager.h@222 PS1, Line 222: bool initial_opening_; Could this be 'const'? http://gerrit.cloudera.org:8080/#/c/21295/1/src/kudu/fs/dir_manager.h@471 PS1, Line 471: // The directories which are opening initially, either when creating the initial file system : // layout, or adding new directories. : std::set<std::string> initial_opening_dirs_; Why not to call this 'created_dirs_' then if it's about actually creating new directories? It would be great to add information on when the contents of this set becomes valid, i.e. when it's legit to use/refer to the elements of the set to check whether any new directories have been created? Is it a requirement to have the elements in the set ordered? http://gerrit.cloudera.org:8080/#/c/21295/1/src/kudu/integration-tests/ts_recovery-itest.cc File src/kudu/integration-tests/ts_recovery-itest.cc: http://gerrit.cloudera.org:8080/#/c/21295/1/src/kudu/integration-tests/ts_recovery-itest.cc@319 PS1, Line 319: fs::kRocksDBDirName Could you please separate this change (i.e. replacing "rdb" string literal with a constant) and other non-functional modifications into its own patch? Thank you! -- 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: 1 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Comment-Date: Fri, 12 Apr 2024 19:06:06 +0000 Gerrit-HasComments: Yes
