-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core: handle sigint (ctrl+c) #1508
base: master
Are you sure you want to change the base?
Conversation
@busbey ^^ |
@busbey ping |
// Handle SIGINT signal (CTRL+C) to export measurements before shutdown | ||
Runtime.getRuntime().addShutdownHook(new Thread(() -> { | ||
if (l.getCount() == 1) { | ||
new TerminatorThread(maxExecutionTime, threads.keySet(), workload).stopWorkload(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you move the TerminatorThread created on line 396 up here can you then reuse the instance? You would still only start the thread on 361 if there is a maxExecutionTime > 0 but always create it for the hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was lazy last time to make stopWorkload
static, so I had to create an instance. This time a made it static.
System.exit(-1); | ||
} | ||
|
||
l.countDown(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a big fan of the multiple l.countDown()
. Ideally there would be only a single call in a finally block but that would require yet another try block and this method is already try block heavy.
I think the nested try blocks on line 378/379 and then again on 405/406 can be merged. Maybe then add an outer try block with all of the exceptions and the finally for the latch? Something like:
try {
// code...
try (final TraceScope span = tracer.newScope(CLIENT_WORKLOAD_SPAN)) {
// code...
}
try (final TraceScope span = tracer.newScope(CLIENT_CLEANUP_SPAN)) {
// code...
}
try (final TraceScope span = tracer.newScope(CLIENT_EXPORT_MEASUREMENTS_SPAN)) {
// code...
}
} catch (WorkloadException e) {
e.printStackTrace();
e.printStackTrace(System.out);
System.exit(0);
} catch (IOException e) {
System.err.println("Could not export measurements, error: " + e.getMessage());
e.printStackTrace();
System.exit(-1);
} finally {
l.countDown();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -325,6 +324,7 @@ public static void main(String[] args) { | |||
} | |||
|
|||
Thread terminator = null; | |||
final CountDownLatch l = new CountDownLatch(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of the CountDownLatch I think obscures what is it doing. I'd suggest moving to a Semaphore.
Here would be:
final Semaphore done = new Semaphore(1);
done.acquire();
Below it would be:
done.release();
Then in the hook:
if( ! done.tryAquire() ) {
// ...stopWorkload()
try {
if( ! done.tryAquire(5, TimeUnit.SECONDS) ) {
System.err.println("Shutdown await timeout. Exit.");
}
} catch (InterruptedException timeout) {
}
}
Extra credit to move this to a Phaser where things can then track the progress of the run (init, workload, cleanup, export measurement). In this case, as an example, you probably don't want to trigger the hook if we are already in the export phase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I am sorry I didnt have time this time to port it to Phaser.
stopWorkload(); | ||
} | ||
|
||
public void stopWorkload() { | ||
System.err.println("Maximum time elapsed. Requesting stop for the workload."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This message should be in the run()
method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
This patch makes YCSB to print measurement results when the process receives a SIGINT (CTRL+C) signal to output results before exit. It is useful for all who don't want to wait for the process to finish requested load amount and don't want to lose measured results.
@allanbank ping |
This patch makes YCSB to print measurement
results when the process receives a SIGINT
(CTRL+C) signal to output results before exit.
It is useful for all who don't want to wait
for the process to finish requested load amount
and don't want to lose measured results.