Zihao Ye 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 12: (2 comments) It looks good, just a minor issue remains. http://gerrit.cloudera.org:8080/#/c/20921/12/package/bin/impala.sh File package/bin/impala.sh: http://gerrit.cloudera.org:8080/#/c/20921/12/package/bin/impala.sh@93 PS12, Line 93: echo "Killed ${service}." && exit 0 Shouldn't we execute the kill command before this? http://gerrit.cloudera.org:8080/#/c/20921/12/package/bin/impala.sh@95 PS12, Line 95: for ((i=1; i<=${counts}; i++)); do : [[ ${i} -gt 1 ]] && sleep ${period} : kill ${pid} : if ! kill -0 ${pid} &> /dev/null; then : rm ${service_pidfile} && echo "(${i}/${counts}) ${service} is stopped." && exit 0 : else : echo "(${i}/${counts}) Waiting ${service} to stop." : fi : done : echo "Timed out waiting ${service} to stop, check logs for more details." nit: I think it would be better to separate this piece of logic into a function. This way, it can be easily reused when we want to support graceful shutdown in the future. If separated, executing the kill command within a loop might be unnecessary. It would be better to let it serve purely as a state check. -- 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: 12 Gerrit-Owner: Xiang Yang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Xiang Yang <[email protected]> Gerrit-Reviewer: Zihao Ye <[email protected]> Gerrit-Comment-Date: Tue, 19 Mar 2024 09:17:41 +0000 Gerrit-HasComments: Yes
