Allow force stopping server process#107170
Allow force stopping server process#107170elasticsearchmachine merged 4 commits intoelastic:mainfrom
Conversation
This commit allows the cli access to sending SIGKILL to the underlying jvm process.
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
mosche
left a comment
There was a problem hiding this comment.
Code looks good, just curious about the intend? Is that meant to support testing abrupt termination?
Some comments on the test, though.
| }; | ||
| var server = startProcess(false, false); | ||
| nonInterruptibleVoid(inMain::await); | ||
| forceStopCallback = blockMain::countDown; |
There was a problem hiding this comment.
Not sure I understand the intention of the forceStopCallback here, nonInterruptibleVoid(blockMain::await) will be interrupted and throw an AssertionError, counting down the latch doesn't make any difference.
There was a problem hiding this comment.
forceStop waits on the process to die. In this test, main is waiting indefinitely. Even though calling forceStop cancels the task, the thread is still alive, waiting on the latch. So without decrementing the latch, while in forceStop, the waitFor would wait indefinitely.
There was a problem hiding this comment.
Even though calling forceStop cancels the task, the thread is still alive, waiting on the latch
Makes sense, though as long as the interrupt status after cancelling the thread isn't lost, blockMain.await() will be interrupted without having to count down. So a noop callback works just as well here.
| forceStopCallback = blockMain::countDown; | ||
| server.forceStop(); | ||
|
|
||
| assertThat(process.main.isDone(), is(true)); // stop should have waited |
There was a problem hiding this comment.
isCancelled instead of isDone to clarify the difference?
|
The intent is to allow forcing the process to stop. While we don't use it in the ServerCli, serverless has a need to quickly shutdown ES in certain circumstances where we don't want to wait for a "clean" shutdown. |
mosche
left a comment
There was a problem hiding this comment.
LGTM, though would be good to use isCancelled as mentioned.
This commit allows the cli access to sending SIGKILL to the underlying jvm process.
This commit allows the cli access to sending SIGKILL to the underlying jvm process.