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 72: Code-Review+2

(4 comments)

Please address one nit about using IsGTest() instead using NO_TESTS macro.

Otherwise, this looks good to me!

http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/CMakeLists.txt
File src/kudu/fs/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/CMakeLists.txt@44
PS71, Line 44: kudu_util
> In https://gerrit.cloudera.org/c/18569/71/src/kudu/fs/dir_manager.cc#221, I
Well, NO_TESTS is rather something about the way the binaries are built, not 
whether some code is running in a test context.

Please consider updating this and using IsGTest() utility function that's 
declared in test_util_prod.h


http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/dir_manager.cc
File src/kudu/fs/dir_manager.cc:

http://gerrit.cloudera.org:8080/#/c/18569/71/src/kudu/fs/dir_manager.cc@207
PS71, Line 207: RdbDir::RdbDir(Env* env, DirMetrics* metrics,
> Because it's a static member, it causes a link error if not define it here.
Ah, indeed -- I missed this.  Thank you for the clarification.


http://gerrit.cloudera.org:8080/#/c/18569/72/src/kudu/fs/dir_manager.cc
File src/kudu/fs/dir_manager.cc:

http://gerrit.cloudera.org:8080/#/c/18569/72/src/kudu/fs/dir_manager.cc@220
PS72, Line 220: #if defined(NO_TESTS)
Please use IsGTest() instead.  You could find its usages in data_dirs.cc and a 
few other places.


http://gerrit.cloudera.org:8080/#/c/18569/71/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:

http://gerrit.cloudera.org:8080/#/c/18569/71/thirdparty/build-definitions.sh@1192
PS71, Line 1192:     -DWITH_LZ4=ON
> I added a new option "-Dlz4_ROOT_DIR=$PREFIX" for RocksDB to find lz4 accor
That's perfect, thank you!



--
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: 72
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: Tue, 30 Jan 2024 01:56:11 +0000
Gerrit-HasComments: Yes

Reply via email to