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 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@58 PS6, Line 58: using namespace apache::thrift; > If you make more changes to this file then you could remove this duplicated Done http://gerrit.cloudera.org:8080/#/c/10847/6/be/src/catalog/catalogd-main.cc@64 PS6, Line 64: InitFeSupport(); > Without your change, what was the exit code when Impala caught a SIGTERM? I'm not able to find what was the exit code when Impala caught a SIGTERM out but as per convention exit code 0 is success/desirable and other exit codes are not desirable/failure. 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 Done 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. Updated the comment to include impalad/statestored/catalogd 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 I have moved it further up as per Todd's comment, log messages won't be printed if I move the code block further up. 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 m Done 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? I tried testing for PID, the problem is that signal is sent from a subprocess so can't get it's 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: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Pranay Singh Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com> Gerrit-Comment-Date: Tue, 07 Aug 2018 21:29:15 +0000 Gerrit-HasComments: Yes
