Alexey Serbin 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 61: (17 comments) http://gerrit.cloudera.org:8080/#/c/18569/61//COMMIT_MSG Commit Message: PS61: I don't remember whether I asked this question before, but have you considered making the rocksdb library dependency not a compile-time, but rather run-time? For example, for C libraries that's usually done using dlopen() and friends. Is that feasible at all? http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/dir_manager.h File src/kudu/fs/dir_manager.h: http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/dir_manager.h@49 PS61, Line 49: is only use internally nit: is used only internally http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/dir_manager.h@109 PS61, Line 109: class Dir Does it make sense to use the sub-classing here, creating a class that inherits from Dir, so only the derived class has the extra member fields and extra methods that are RocksDB-only? http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/dir_manager.h@121 PS61, Line 121: return nit: returns http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/dir_manager.h@199 PS61, Line 199: variables nit: fields http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/dir_manager.h@201 PS61, Line 201: , nit: . http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/dir_manager.h@308 PS61, Line 308: // Finds a directory by full path name, returning null if it can't be found. : // : // NOTE: Only for test purpose. : Dir* FindDirByFullPathForTests(const std::string& full_path) const; If this is test-only, is it feasible to move this into the protected/private section, and introduce corresponding friendship for particular tests? http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/dir_manager.cc File src/kudu/fs/dir_manager.cc: http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/dir_manager.cc@134 PS61, Line 134: CHECK_STREQ It seems DCHECK_STREQ() is used for the same invariant check on line 187. Does it make sense to switch to DCHECK here as well? >From the other side, maybe these checks will not be necessary at all of using >using sub-classing for the Dir class to have a class that has RocksDB >specifics? http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/log_block_manager-test-util.cc File src/kudu/fs/log_block_manager-test-util.cc: http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/log_block_manager-test-util.cc@626 PS61, Line 626: CHECK Why CHECK() here and DCHECK() elsewhere? Is it possible to switch to DCHECK() everywhere? 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@420 PS61, Line 420: Some initialize work Could you be more specific what exactly is supposed to be initialized by this method? http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/log_block_manager.h@599 PS61, Line 599: The data in RocksDB will be compacted BTW, what happens if a tablet server shuts down while the RocksDB background compaction is running? Could it bring any inconsistency for the RocksDB's data? http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/log_block_manager.h@599 PS61, Line 599: background in background http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/fs/log_block_manager.h@657 PS61, Line 657: repairing a fatal inconsistency failed failing to repair an inconsistency BTW, what's difference between 'inconsistency' and 'fatal inconsistency'? 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: kEncryptionHeaderSize nit: why is this name for the variable that's not a static constant? http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/tserver/tablet_server-test.cc@763 PS61, Line 763: will start fail will fail starting http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/tserver/tablet_server-test.cc@763 PS61, Line 763: failed of the failure http://gerrit.cloudera.org:8080/#/c/18569/61/src/kudu/tserver/tablet_server-test.cc@764 PS61, Line 764: cases case -- 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: 61 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: Fri, 22 Dec 2023 23:36:30 +0000 Gerrit-HasComments: Yes
