Lars Volker has posted comments on this change. ( http://gerrit.cloudera.org:8080/12973 )
Change subject: IMPALA-8401: SIGRTMIN initiates the graceful shutdown process ...................................................................... Patch Set 3: (15 comments) http://gerrit.cloudera.org:8080/#/c/12973/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12973/3//COMMIT_MSG@11 PS3, Line 11: by sending SIGRTMIN signal to it. Maybe include a sample usage "kill -SIGRTMIN <PID>" in the commit message? http://gerrit.cloudera.org:8080/#/c/12973/3/be/src/common/init.h File be/src/common/init.h: http://gerrit.cloudera.org:8080/#/c/12973/3/be/src/common/init.h@24 PS3, Line 24: // Using the first real-time signal available to initiate graceful shutdown. Mention "man 7 signal" in the comment to point to further information? http://gerrit.cloudera.org:8080/#/c/12973/3/be/src/common/init.cc File be/src/common/init.cc: http://gerrit.cloudera.org:8080/#/c/12973/3/be/src/common/init.cc@183 PS3, Line 183: int signal; nit: move closer to where it's used http://gerrit.cloudera.org:8080/#/c/12973/3/be/src/common/init.cc@194 PS3, Line 194: static This doesn't need to be static and confused me, maybe just make it const int? http://gerrit.cloudera.org:8080/#/c/12973/3/be/src/common/init.cc@197 PS3, Line 197: Quit Shutdown signal received, here and below. http://gerrit.cloudera.org:8080/#/c/12973/3/be/src/common/init.cc@234 PS3, Line 234: sigemptyset(&signals); CHECK_EQ(0, ...) like above? Or handle errors properly and print some message? http://gerrit.cloudera.org:8080/#/c/12973/3/be/src/common/init.cc@239 PS3, Line 239: IMPALA_QUIT_SIGNAL IMPALA_SHUTDOWN_SIGNAL http://gerrit.cloudera.org:8080/#/c/12973/3/be/src/common/init.cc@391 PS3, Line 391: quit shutdown-signal-handler http://gerrit.cloudera.org:8080/#/c/12973/3/be/src/service/impalad-main.cc File be/src/service/impalad-main.cc: http://gerrit.cloudera.org:8080/#/c/12973/3/be/src/service/impalad-main.cc@95 PS3, Line 95: ABORT_IF_ERROR(StartImpalaShutdownSignalHandlerThread()); I think this could be further, e.g. after InitCommonRuntime, because nothing depends on it and it only depends on that call. Let's not put it between server->Start and server->Join. http://gerrit.cloudera.org:8080/#/c/12973/3/tests/custom_cluster/test_restart_services.py File tests/custom_cluster/test_restart_services.py: http://gerrit.cloudera.org:8080/#/c/12973/3/tests/custom_cluster/test_restart_services.py@433 PS3, Line 433: def test_shutdown_signal(self): What happens if we send another signal while the daemon is already shutting down? http://gerrit.cloudera.org:8080/#/c/12973/3/tests/custom_cluster/test_restart_services.py@434 PS3, Line 434: startup shutdown grace period? http://gerrit.cloudera.org:8080/#/c/12973/3/tests/custom_cluster/test_restart_services.py@437 PS3, Line 437: LOG.info("Sending signal to impalad...") Maybe include the PID in the log message (and the value of SIGRTMIN for good measure)? http://gerrit.cloudera.org:8080/#/c/12973/3/tests/custom_cluster/test_restart_services.py@438 PS3, Line 438: impalad.send_signal(signal.SIGRTMIN) Maybe add a constant IMPALA_SHUTDOWN_SIGNAL at the top of the file? http://gerrit.cloudera.org:8080/#/c/12973/3/tests/custom_cluster/test_restart_services.py@445 PS3, Line 445: 10 Why 10? http://gerrit.cloudera.org:8080/#/c/12973/3/tests/custom_cluster/test_restart_services.py@448 PS3, Line 448: Quit Shutdown signal received? -- 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: 3 Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 12 Apr 2019 23:27:05 +0000 Gerrit-HasComments: Yes
