Lars Volker 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 8: (7 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@64 PS6, Line 64: InitFeSupport(); > This exit will ensure a normal process termination, if I don't use it the p Without your change, what was the exit code when Impala caught a SIGTERM? http://gerrit.cloudera.org:8080/#/c/10847/8/be/src/common/init.cc File be/src/common/init.cc: http://gerrit.cloudera.org:8080/#/c/10847/8/be/src/common/init.cc@177 PS8, Line 177: // doing an exit. nit: wrap at 90c, please also check the rest of the change http://gerrit.cloudera.org:8080/#/c/10847/8/be/src/common/init.cc@291 PS8, Line 291: catalogd This does not seem specific to the catalog. http://gerrit.cloudera.org:8080/#/c/10847/8/be/src/common/init.cc@291 PS8, Line 291: // Signal handler for handling the SIGTERM. We want to log a message when catalogd is : // being shutdown using a SIGTERM. : struct sigaction action; : memset(&action, 0, sizeof(struct sigaction)); : action.sa_sigaction = &HandleSigTerm; : sigaction(SIGTERM, &action, nullptr); Can this whole block be moved to earlier in the initialization? It takes a while for everything to get initialized and if we catch a SIGTERM during that time we won't see any output (see Todd's earlier comment). http://gerrit.cloudera.org:8080/#/c/10847/8/be/src/util/minidump.cc File be/src/util/minidump.cc: http://gerrit.cloudera.org:8080/#/c/10847/8/be/src/util/minidump.cc@101 PS8, Line 101: LOG(INFO) << "Caught signal: SIGUSR1" << endl; This should get the signal from the parameter. You could use the same log message format that you use for SIGTERM here, too. 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 signal: SIGTERM. Daemon will exit', > Checked the code for exit of the process, could not find the function where Can you think of ways that we could add this to the code? Maybe we can pass an expected exit code to kill_cluster()? http://gerrit.cloudera.org:8080/#/c/10847/8/tests/custom_cluster/test_breakpad.py File tests/custom_cluster/test_breakpad.py: http://gerrit.cloudera.org:8080/#/c/10847/8/tests/custom_cluster/test_breakpad.py@239 PS8, Line 239: self.assert_impalad_log_contains('INFO', 'Caught signal: SIGTERM. Daemon will exit', Can you test that the log also contains the sending PID? -- 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: 8 Gerrit-Owner: Pranay Singh Gerrit-Reviewer: Impala Public Jenkins <[email protected]> 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: Mon, 06 Aug 2018 17:38:18 +0000 Gerrit-HasComments: Yes
