[Impala-ASF-CR] IMPALA-6271: Impala daemon should log a message when it's being shut down

["Pranay Singh (Code Review)" <ge...@cloudera.org>]

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 <lv...@cloudera.com>
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Fri, 03 Aug 2018 23:29:51 +0000
Gerrit-HasComments: Yes