Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/12973 )
Change subject: IMPALA-8401: SIGQUIT initiates the graceful shutdown process ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/12973/1/be/src/service/impalad-main.cc File be/src/service/impalad-main.cc: http://gerrit.cloudera.org:8080/#/c/12973/1/be/src/service/impalad-main.cc@62 PS1, Line 62: boost::shared_ptr<ImpalaServer> impala_server; What are the implications of moving this here on the lifetime of impala_server? Before this change it got destructed at the end of ImpaladMain, now it will happen during process shutdown. If we want to keep it, we can go with the previous approach and store an additional pointer here that we set back to nullptr before exiting ImpaladMain below. http://gerrit.cloudera.org:8080/#/c/12973/1/be/src/service/impalad-main.cc@69 PS1, Line 69: Status status = impala_server->StartShutdown(ONE_YEAR_IN_SECONDS, &shutdown_status); StartShutdown creates a new thread, which might not be re-entrant safe. If we're in the process of handling a client-issued shutdown, then catch this signal, we will possibly interrupt its execution (e.g. the thread creation) and find its global structures in an inconsistent state. We probably should limit this handler to setting a flag, and then initiate the shutdown itself from a separate thread outside the handler. See IMPALA-4796 for a similar issue and kudu/util/minidump.h as an example. http://gerrit.cloudera.org:8080/#/c/12973/1/be/src/service/impalad-main.cc@71 PS1, Line 71: LOG(ERROR) << "Quit signal received but unable to initiate shutdown. Status: " I think these LOG macros end up not being async-signal safe. In minidump.cc we use a wrapper around the write syscall to log during signal handlers. http://gerrit.cloudera.org:8080/#/c/12973/1/be/src/service/impalad-main.cc@116 PS1, Line 116: sigaction You should probably memset this to 0 before using it. See minidump.cc for an example. -- To view, visit http://gerrit.cloudera.org:8080/12973 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I521ffd7526ac9a8a5c4996994eb68d6a855aef86 Gerrit-Change-Number: 12973 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 09 Apr 2019 23:19:56 +0000 Gerrit-HasComments: Yes
