Quanlong Huang has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/20921 )

Change subject: IMPALA-12362: (part-1/4) Refactor service management scripts.
......................................................................


Patch Set 7:

(8 comments)

Tried deploying Impala in a CM managed cluster. It's cool to be able to specify 
the flags in the commandline!

http://gerrit.cloudera.org:8080/#/c/20921/7/package/bin/impala.sh
File package/bin/impala.sh:

http://gerrit.cloudera.org:8080/#/c/20921/7/package/bin/impala.sh@21
PS7, Line 21: custom
nit: "customize"


http://gerrit.cloudera.org:8080/#/c/20921/7/package/bin/impala.sh@22
PS7, Line 22: custom
nit: "customize"


http://gerrit.cloudera.org:8080/#/c/20921/7/package/bin/impala.sh@47
PS7, Line 47:   if ps -p ${pid} &> /dev/null ; then
This is not enough since the PID can be reused by other process after the 
server crashed.

We need to double check if the service is running with PID, e.g. as the 
original script does:

    if ! ps $pid | grep $service > /dev/null 2>&1; then
      echo "$service with PID $pid doesn't exist"
      break
    fi

We also need a method to return whether the service is ready. E.g. catalogd 
could take time to load the db/table lists at startup, and impalad is not ready 
until it receive the initial catalog update. We've seen it takes 1~2 hours in a 
large warehouse. Such an un-ready state could take longer if catalogd runs with 
--load_catalog_in_background=true. Can we find a place to keep the ready check?
https://github.com/apache/impala/blob/eaa35b02507a834edd0d219343fd4bd075f21762/package/bin/impala-env.sh#L65C1-L73C7


http://gerrit.cloudera.org:8080/#/c/20921/7/package/bin/impala.sh@50
PS7, Line 50:   else
            :     echo "${service} is stopped."
            :     exit 1
            :   fi
nit: move the content of the else-branch outside since the if-branch always 
exits.

  if ps -p ${pid} &> /dev/null ; then
    echo "${service} is running with PID ${pid}."
    exit 0
  fi
  echo "${service} is stopped."
  exit 1


http://gerrit.cloudera.org:8080/#/c/20921/7/package/bin/impala.sh@86
PS7, Line 86: ${HADOOP_HOME}
Use "${HADOOP_HOME:=}" in case it's not set.


http://gerrit.cloudera.org:8080/#/c/20921/7/package/bin/impala.sh@97
PS7, Line 97: ${LIBHDFS_OPTS}
Use "${LIBHDFS_OPTS:-}" in case it's not set.


http://gerrit.cloudera.org:8080/#/c/20921/7/package/bin/start-impalad.sh
File package/bin/start-impalad.sh:

http://gerrit.cloudera.org:8080/#/c/20921/7/package/bin/start-impalad.sh@a26
PS7, Line 26:
            :
Can we add a similar guide somewhere for running with another username?

In a CM managed cluster, user 'root' is not allowed to access the hive 
warehouse by default.


http://gerrit.cloudera.org:8080/#/c/20921/7/package/conf/statestored_flags
File package/conf/statestored_flags:

http://gerrit.cloudera.org:8080/#/c/20921/7/package/conf/statestored_flags@5
PS7, Line 5: webserver_doc_root
We can remove "webserver_doc_root" as you suggested at 
https://gerrit.cloudera.org/c/20263/6/package/conf/catalogd_flags#b9



--
To view, visit http://gerrit.cloudera.org:8080/20921
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f4dcad9cfa12d351d562e7ef8c0a8957d3ca147
Gerrit-Change-Number: 20921
Gerrit-PatchSet: 7
Gerrit-Owner: Xiang Yang <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Quanlong Huang <[email protected]>
Gerrit-Comment-Date: Tue, 30 Jan 2024 08:19:41 +0000
Gerrit-HasComments: Yes

Reply via email to