Bikramjeet Vig 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? Done 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? Done 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 Done 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 in Done http://gerrit.cloudera.org:8080/#/c/12973/3/be/src/common/init.cc@197 PS3, Line 197: Quit > Shutdown signal received, here and below. Done 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 messa Done. cannot call CHECK_EQ though as InitGoogleLoggingSafe has not been called yet. http://gerrit.cloudera.org:8080/#/c/12973/3/be/src/common/init.cc@239 PS3, Line 239: IMPALA_QUIT_SIGNAL > IMPALA_SHUTDOWN_SIGNAL Done http://gerrit.cloudera.org:8080/#/c/12973/3/be/src/common/init.cc@391 PS3, Line 391: quit > shutdown-signal-handler Done 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 nothin I had added a pre-condition to this method to call it after impala-server has started. it ideally does not depend on it and can do away with that dependency by re-adding the checks i removed in my patch set 2 but just having those makes the code a little simpler. Please let me know if you still feel I should bring back those checks, remove the related pre-condition, and move this after InitCommonRuntime(). 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 added a test for this http://gerrit.cloudera.org:8080/#/c/12973/3/tests/custom_cluster/test_restart_services.py@434 PS3, Line 434: startup > shutdown grace period? Done 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 goo Done 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? Done http://gerrit.cloudera.org:8080/#/c/12973/3/tests/custom_cluster/test_restart_services.py@445 PS3, Line 445: 10 > Why 10? No specific reason, as mentioned in the comment above, it is just for a margin of error to avoid flakiness. http://gerrit.cloudera.org:8080/#/c/12973/3/tests/custom_cluster/test_restart_services.py@448 PS3, Line 448: Quit > Shutdown signal received? Done -- 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 <[email protected]> Gerrit-Reviewer: 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: Mon, 15 Apr 2019 20:32:07 +0000 Gerrit-HasComments: Yes
