[Impala-ASF-CR] WIP IMPALA-12362: (part-4/4) Refactor linux packaging related cmake files.

2024-04-07 Thread Quanlong Huang (Code Review)
Quanlong Huang 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:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20263/7/package/CMakeLists.txt
File package/CMakeLists.txt:

http://gerrit.cloudera.org:8080/#/c/20263/7/package/CMakeLists.txt@39
PS7, Line 39: FILE(GLOB gcc_lib ${IMPALA_GCC_HOME}/lib64/libgcc_s.so.1*)
: install(FILES ${gcc_lib} DESTINATION 
${IMPALA_INSTALLDIR}/lib/native)
:
: FILE(GLOB cpp_lib ${IMPALA_GCC_HOME}/lib64/libstdc++.so.6*)
: install(FILES ${cpp_lib} DESTINATION 
${IMPALA_INSTALLDIR}/lib/native)
:
: set(KUDU_HOME 
$ENV{IMPALA_TOOLCHAIN_PACKAGES_HOME}/kudu-$ENV{IMPALA_KUDU_VERSION}/release)
: # The parent folder is lib64 on centos/redhat, while on ubuntu 
it's lib.
: FILE(GLOB kudu_lib ${KUDU_HOME}/lib*/libkudu_client.so*)
: install(FILES ${kudu_lib} DESTINATION 
${IMPALA_INSTALLDIR}/lib/native)
:
: FILE(GLOB hadoop_lib $ENV{HADOOP_LIB_DIR}/native/libhadoop.so*)
: install(FILES ${hadoop_lib} DESTINATION 
${IMPALA_INSTALLDIR}/lib/native)
Can we add a check that if any of these files are not found, exit the build? I 
encountered such issue when backporting the packaging support to older branches.



--
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 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Xiang Yang 
Gerrit-Comment-Date: Mon, 08 Apr 2024 05:28:00 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] WIP IMPALA-12362: (part-4/4) Refactor linux packaging related cmake files.

2024-02-03 Thread Xiang Yang (Code Review)
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:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/20263/7//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/20263/7//COMMIT_MSG@9
PS7, Line 9: independent linux packaging related content to 
package/CMakeLists.txt.
> Could you share the benifits of doing this?
the main reason is that I want to make the cmake file structure more clearly :)



--
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 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Xiang Yang 
Gerrit-Comment-Date: Sat, 03 Feb 2024 10:50:58 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] WIP IMPALA-12362: (part-4/4) Refactor linux packaging related cmake files.

2024-01-30 Thread Quanlong Huang (Code Review)
Quanlong Huang 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:

(4 comments)

Thanks for spliting the patch!

http://gerrit.cloudera.org:8080/#/c/20263/7//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/20263/7//COMMIT_MSG@9
PS7, Line 9: independent linux packaging related content to 
package/CMakeLists.txt.
Could you share the benifits of doing this?


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@49
PS6, Line 49:
> what do you mean? remove the else branch, or keep the content out of the if
Replied at https://gerrit.cloudera.org/c/20921/7/package/bin/impala.sh#53


http://gerrit.cloudera.org:8080/#/c/20263/6/package/bin/impala.sh@109
PS6, Line 109:
> Generally service startup tools (such as systemd, or the startup script for
Let's continue the discussion at 
https://gerrit.cloudera.org/c/20921/7/package/bin/impala.sh#47


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:
> I don't want to write dead a path in default configurations, I found that '
Ack



--
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 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Xiang Yang 
Gerrit-Comment-Date: Tue, 30 Jan 2024 08:22:12 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] WIP IMPALA-12362: (part-4/4) Refactor linux packaging related cmake files.

2024-01-20 Thread Impala Public Jenkins (Code Review)
Impala Public Jenkins 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:

Build Successful

https://jenkins.impala.io/job/gerrit-code-review-checks/15009/ : Initial code 
review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun 
to run full precommit tests.


--
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 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Xiang Yang 
Gerrit-Comment-Date: Sat, 20 Jan 2024 13:21:46 +
Gerrit-HasComments: No


[Impala-ASF-CR] WIP IMPALA-12362: (part-4/4) Refactor linux packaging related cmake files.

2024-01-20 Thread Xiang Yang (Code Review)
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 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Quanlong Huang 
Gerrit-Reviewer: Xiang Yang 
Gerrit-Comment-Date: Sat, 20 Jan 2024 12:59:06 +
Gerrit-HasComments: Yes


[Impala-ASF-CR] WIP IMPALA-12362: (part-4/4) Refactor linux packaging related cmake files.

2024-01-20 Thread Xiang Yang (Code Review)
Hello Quanlong Huang, Laszlo Gaal, Impala Public Jenkins,

I'd like you to reexamine a change. Please visit

http://gerrit.cloudera.org:8080/20263

to look at the new patch set (#7).

Change subject: WIP IMPALA-12362: (part-4/4) Refactor linux packaging related 
cmake files.
..

WIP IMPALA-12362: (part-4/4) Refactor linux packaging related cmake files.

independent linux packaging related content to package/CMakeLists.txt.

Testing:
 - Manually deploy package on Ubuntu22.04 and verify it.

Change-Id: If3914dcda69f81a735cdf70d76c59fa09454777b
---
M .gitignore
M CMakeLists.txt
A package/CMakeLists.txt
3 files changed, 133 insertions(+), 106 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/63/20263/7
--
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: newpatchset
Gerrit-Change-Id: If3914dcda69f81a735cdf70d76c59fa09454777b
Gerrit-Change-Number: 20263
Gerrit-PatchSet: 7
Gerrit-Owner: Xiang Yang 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Quanlong Huang