Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sitano
Copy link
Contributor

@sitano sitano commented Jan 27, 2021

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.

@sitano
Copy link
Contributor Author

sitano commented Jan 27, 2021

@busbey ^^
example_output.txt
It was useful for me when I was running tests. Decided to contribute with CTRL+C handler.

@sitano
Copy link
Contributor Author

sitano commented Feb 26, 2021

@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();
Copy link
Collaborator

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.

Copy link
Contributor Author

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();
Copy link
Collaborator

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();
}

Copy link
Contributor Author

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);
Copy link
Collaborator

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.

Copy link
Contributor Author

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.");
Copy link
Collaborator

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.

Copy link
Contributor Author

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.
@sitano
Copy link
Contributor Author

sitano commented Mar 28, 2021

@allanbank ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants