Xiang Yang has posted comments on this change. ( http://gerrit.cloudera.org:8080/20263 )
Change subject: WIP IMPALA-12362: (part-4/4) Refactor linux packaging related cmake files. ...................................................................... Patch Set 7: (8 comments) > Patch Set 6: > > (8 comments) > > Thanks for improving the packaging support! There are several independent > topics in this patch, e.g. CMake files refactoring, scripts refactoring, > default configuration changes, adding more binaries, etc. To be easier for > review, It'd be nice to split this into several smaller patches. Hi quanlong, thanks for your review! I've splitted the original patch into 4 smaller patches: https://gerrit.cloudera.org/c/20921/ https://gerrit.cloudera.org/c/20928/ https://gerrit.cloudera.org/c/20929/ https://gerrit.cloudera.org/c/20263/ (self) There are dependencies between them, so you can review them one by one. http://gerrit.cloudera.org:8080/#/c/20263/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20263/6//COMMIT_MSG@18 PS6, Line 18: > It'd be nice to also add impala-profile-tool so users can parse the thrift Done in https://gerrit.cloudera.org/c/20929/ http://gerrit.cloudera.org:8080/#/c/20263/6/package/CMakeLists.txt File package/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/20263/6/package/CMakeLists.txt@28 PS6, Line 28: install(TARGETS impalad DESTINATION ${IMPALA_INSTALLDIR}/sbin) : : install(FILES ${CMAKE_SOURCE_DIR}/LICENSE.txt DESTINATION ${IMPALA_INSTALLDIR}) : install(DIRECTORY "${CMAKE_SOURCE_DIR}/www/" DESTINATION ${IMP > We already have these in be/src/service/CMakeLists.txt. Do we need to dupli Done http://gerrit.cloudera.org:8080/#/c/20263/6/package/bin/impala.sh File package/bin/impala.sh: http://gerrit.cloudera.org:8080/#/c/20263/6/package/bin/impala.sh@21 PS6, Line 21: > nit: customize Done in https://gerrit.cloudera.org/c/20921/ http://gerrit.cloudera.org:8080/#/c/20263/6/package/bin/impala.sh@22 PS6, Line 22: > nit: customize same as above http://gerrit.cloudera.org:8080/#/c/20263/6/package/bin/impala.sh@49 PS6, Line 49: > nit: don't need "else" what do you mean? remove the else branch, or keep the content out of the if logical block? http://gerrit.cloudera.org:8080/#/c/20263/6/package/bin/impala.sh@109 PS6, Line 109: > Any reason for removing the logic of wait_for_ready? I think it's helpful w Generally service startup tools (such as systemd, or the startup script for apache doris) won't block until the service is available, but split the health check function into separate commands, so I add a 'status' subcommand to check the service health. The reason for only checking if the process is alive is that the webserver may be disabled, thus checking the '/healthz' interface is not a precise way to do it. http://gerrit.cloudera.org:8080/#/c/20263/6/package/conf/catalogd_flags File package/conf/catalogd_flags: http://gerrit.cloudera.org:8080/#/c/20263/6/package/conf/catalogd_flags@a9 PS6, Line 9: > Why do we remove this? Without the correct doc root, the webUI might not be I don't want to write dead a path in default configurations, I found that 'webserver_doc_root' default to $IMPALA_HOME: https://github.com/apache/impala/blob/4.3.0/be/src/util/webserver.cc#L187, so I export IMPALA_HOME variable in https://gerrit.cloudera.org/c/20928. http://gerrit.cloudera.org:8080/#/c/20263/6/package/conf/catalogd_flags@20 PS6, Line 20: > We should set -v=1 explicitly. Otherwise, no INFO logs will be shown. The d Done in https://gerrit.cloudera.org/c/20928/ -- To view, visit http://gerrit.cloudera.org:8080/20263 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If3914dcda69f81a735cdf70d76c59fa09454777b Gerrit-Change-Number: 20263 Gerrit-PatchSet: 7 Gerrit-Owner: Xiang Yang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Laszlo Gaal <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Xiang Yang <[email protected]> Gerrit-Comment-Date: Sat, 20 Jan 2024 12:59:06 +0000 Gerrit-HasComments: Yes
