Pranay Singh has posted comments on this change. ( http://gerrit.cloudera.org:8080/10847 )
Change subject: IMPALA-6271: Impala daemon should log a message when it's being shut down ...................................................................... Patch Set 6: (9 comments) http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/catalog/catalogd-main.cc File be/src/catalog/catalogd-main.cc: http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/catalog/catalogd-main.cc@62 PS6, Line 62: HandlerTerm > "HandleSigTerm"? Done http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/catalog/catalogd-main.cc@63 PS6, Line 63: LOG(INFO) << "Caught SIGTERM daemon going down" << endl; > nit: please format this more grammatically. For example "Caught SIGTERM. Da Changed the log message and removed the endl , looks like the messages are being flushed perhaps by atexit hook as you say. Since the testcase check for the message "Caught SIGTERM" which seems to be working fine. http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/catalog/catalogd-main.cc@64 PS6, Line 64: exit(0); > Does this change the behavior? I.e., would we have exited with 0 before, to This exit will ensure a normal process termination, if I don't use it the process will not be terminated. http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/catalog/catalogd-main.cc@120 PS6, Line 120: // We want to log a message when catalogd is > nit: wrap comments at 90 chars, here and elsewhere. Done http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/catalog/catalogd-main.cc@125 PS6, Line 125: sigaction(SIGTERM, &action, nullptr); > why not set this earlier? seems like there could be a relatively long start Done http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/service/impalad-main.cc File be/src/service/impalad-main.cc: http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/service/impalad-main.cc@60 PS6, Line 60: // doing an exit. > This code is now copy-pasted to several files. Can you dedup it? Removed the code block and moved it to the common handler http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/service/impalad-main.cc@104 PS6, Line 104: // We want to log a message when impalad is > same here Done http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/util/minidump.cc File be/src/util/minidump.cc: http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/util/minidump.cc@101 PS6, Line 101: LOG(INFO) << "Signal received SIGUSR1" << endl; > The other messages are "Caught SIGTERM daemon going down". Can you make the Done http://gerrit.cloudera.org:8080/#/c/10847/6/tests/custom_cluster/test_breakpad.py File tests/custom_cluster/test_breakpad.py: http://gerrit.cloudera.org:8080/#/c/10847/6/tests/custom_cluster/test_breakpad.py@239 PS6, Line 239: self.assert_impalad_log_contains('INFO', 'Caught SIGTERM daemon going down', > Is there a way to also check the exit code of the processes? Checked the code for exit of the process, could not find the function where the exit of the process is checked. -- To view, visit http://gerrit.cloudera.org:8080/10847 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id20da9e30440b7348557beccb8a0da14775fcc29 Gerrit-Change-Number: 10847 Gerrit-PatchSet: 6 Gerrit-Owner: Pranay Singh Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Pranay Singh Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Zoram Thanga <[email protected]> Gerrit-Comment-Date: Fri, 03 Aug 2018 23:29:51 +0000 Gerrit-HasComments: Yes
