Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18770 )
Change subject: KUDU-3374 Add support for M1 and macOS Monterey ...................................................................... Patch Set 11: (25 comments) http://gerrit.cloudera.org:8080/#/c/18770/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18770/11//COMMIT_MSG@26 PS11, Line 26: consisted consistent ? http://gerrit.cloudera.org:8080/#/c/18770/11//COMMIT_MSG@30 PS11, Line 30: Added a patch to fix null pointer dereference in rapidjson. : [6] how about: Added a patch [6] to fix null pointer dereference in rapidjson. http://gerrit.cloudera.org:8080/#/c/18770/11//COMMIT_MSG@32 PS11, Line 32: fix drop 'fix' ? http://gerrit.cloudera.org:8080/#/c/18770/11//COMMIT_MSG@31 PS11, Line 31: Added another patch containing assertions to a similar issue, to : fix suppress clang warnings in rapidjson. [7] How about: Added another patch [7] containing ... http://gerrit.cloudera.org:8080/#/c/18770/11//COMMIT_MSG@32 PS11, Line 32: Building tests in glog : has to be turned off as it causes linker error in tsan build. [8] How about: Building tests in glog has to be turned off [8] as it causes ... http://gerrit.cloudera.org:8080/#/c/18770/11//COMMIT_MSG@36 PS11, Line 36: race races http://gerrit.cloudera.org:8080/#/c/18770/11//COMMIT_MSG@43 PS11, Line 43: [1] Change-Id: I21abd1749fdfdcde412f5a2ca2245c42da20d4f3 : [2] Change-Id: Idc5721cb2445303b2e79d08b547e746929c7486d : [3] Change-Id: I029a858d2da77cea84e7e6856b8a5ac02713152d : [4] Change-Id: Id9c32abe256978158617a4fe3a3c34e9bfd00fb2 Could you change this to the corresponding gerrit review links? http://gerrit.cloudera.org:8080/#/c/18770/11/CMakeLists.txt File CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/18770/11/CMakeLists.txt@168 PS11, Line 168: should be compiled with clang built in thirdparty directory Why? What has changed in this patch so that now we explicitly use LLVM instead of system compiler to build Kudu? http://gerrit.cloudera.org:8080/#/c/18770/11/CMakeLists.txt@170 PS11, Line 170: Because that is the process for building llvm/clang as well. I'm not sure I understand the reasoning here. Could you please rephrases this, maybe? http://gerrit.cloudera.org:8080/#/c/18770/11/cmake_modules/FindGLog.cmake File cmake_modules/FindGLog.cmake: http://gerrit.cloudera.org:8080/#/c/18770/11/cmake_modules/FindGLog.cmake@31 PS11, Line 31: if (APPLE) : set(CMAKE_FIND_LIBRARY_SUFFIXES ".dylib") : else() : set(CMAKE_FIND_LIBRARY_SUFFIXES ".so") : endif() Could you add a comment explaining why to override the default settings for CMAKE_FIND_LIBRARY_SUFFIXES here? http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/common/row_operations-test.cc File src/kudu/common/row_operations-test.cc: http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/common/row_operations-test.cc@302 PS11, Line 302: reinterpret_cast Could this be static_cast<> here as well similar to line 330? http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/security/test/mini_kdc.cc File src/kudu/security/test/mini_kdc.cc: http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/security/test/mini_kdc.cc@108 PS11, Line 108: arm Is it indeed something related to the architecture, not the version of homebrew? http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/server/diagnostics_log.cc File src/kudu/server/diagnostics_log.cc: http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/server/diagnostics_log.cc@21 PS11, Line 21: nit: remove the extra empty line http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/tablet/concurrent_btree.h File src/kudu/tablet/concurrent_btree.h: http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/tablet/concurrent_btree.h@96 PS11, Line 96: if (addr) { Isn't it a programming error if this addr is nullptr? If so, maybe add DCHECK() instead? http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/util/async_logger.h File src/kudu/util/async_logger.h: PS11: Could you separate this and the corresponding changes in async_logger.cc into its own changelist, please? http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/util/logging.h File src/kudu/util/logging.h: http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/util/logging.h@21 PS11, Line 21: nit: remove the extra line http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/util/pstack_watcher.h File src/kudu/util/pstack_watcher.h: PS11: Could you separate changes in this and in the corresponding cc file this into its own changelist? http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/util/pstack_watcher.cc File src/kudu/util/pstack_watcher.cc: http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/util/pstack_watcher.cc@20 PS11, Line 20: #include <cerrno> nit: move this into the C++ headers section http://gerrit.cloudera.org:8080/#/c/18770/11/src/kudu/util/pstack_watcher.cc@170 PS11, Line 170: nit: wrong indent http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/build-definitions.sh@741 PS11, Line 741: --with-openssl Why is this needed? IIRC, this flag is needed to specify custom location of the OpenSSL root directory, and curl is configured with OpenSSL support by default. http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/build-thirdparty.sh File thirdparty/build-thirdparty.sh: http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/build-thirdparty.sh@183 PS11, Line 183: for homebrew_openssl_dir in "${homebrew_openssl_dirs[@]}"; do : if [ -d $homebrew_openssl_dir ]; then : OPENSSL_CFLAGS="-I$homebrew_openssl_dir/include" : OPENSSL_LDFLAGS="-L$homebrew_openssl_dir/lib" : fi What if both directories are present? Is the end result deterministic? Also, what if curl config picks up some other directory for OpenSSL during its auto-detection process? These flags and the auto-detection of OpenSSL are independent, IIRC. http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/download-thirdparty.sh File thirdparty/download-thirdparty.sh: http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/download-thirdparty.sh@a310 PS11, Line 310: I didn't see this file removed among the list of updated files. Also, why to remove it? Is the new version of curl correctly handles the situation with multiple OpenSSL versions present? http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/download-thirdparty.sh@173 PS11, Line 173: 0 Why 0? Usually, this stands for the number of patches applied. http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/preflight.py File thirdparty/preflight.py: http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/preflight.py@141 PS11, Line 141: homebrew_openssl_dirs=["/usr/local/opt/openssl/include", "/opt/homebrew/opt/[email protected]/include"] : for homebrew_openssl_dir in homebrew_openssl_dirs: : if os.path.isdir(homebrew_openssl_dir): If this a part of curl-related changes? If so, could you separate this into the corresponding separate changelist? http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/vars.sh File thirdparty/vars.sh: http://gerrit.cloudera.org:8080/#/c/18770/11/thirdparty/vars.sh@125 PS11, Line 125: CURL_VERSION=7.80.0 Why to upgrade curl as well? Could you move this into its own separate changelist? -- To view, visit http://gerrit.cloudera.org:8080/18770 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9877f95340b969308c317a6bac50665ff78e329e Gerrit-Change-Number: 18770 Gerrit-PatchSet: 11 Gerrit-Owner: Marton Greber <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan <[email protected]> Gerrit-Comment-Date: Wed, 27 Jul 2022 23:56:38 +0000 Gerrit-HasComments: Yes
