broadgsa/gatk-protected — pull request #4 by nc6 (2014-01-17 14:01:39+00:00)
Title: Checkpoint

This is a patch that fixes some issues with running GATK under Berkeley checkpoint/restart, as mentioned at http://gatkforums.broadinstitute.org/discussion/3606.

A quick discussion of the issue and fix:
The current timer uses JVM internal System.nanoTime. This, it seems, can get reset when restarting a checkpoint, which can cause the following issues:

1. Sometimes the origin point for the restarted JVM is far in advance of the current time as held by the timer, and hence the timer reports negative elapsed time. This violates a guard in `GenomeAnalysisEngine.java`.
2. Conversely, sometimes the origin point moves into the past, and thus the job appears to have been running for days or years. This is less of an issue, but can result in confusing statistics and premature termination if GATK has set a runtime limit.

The fix addresses this in two parts:
1. Moving the check in `GenomeAnalysisEngine.java` to occur only if a runtime limit is set - thus if there is no runtime limit the error should not occur.
2. Adding a second timer (`CheckpointableTimer`) suitable for use in checkpoint/restart situations. The second timer is very similar to `SimpleTimer` but also attempts to keep track of the offset between system time and JVM time. If this offset drifts by more than a few seconds from its starting point, it is assumed that a checkpoint/restart has been performed. The current time period is dropped and the offset monitor is reset using the new system and nano times. This hopefully reflects the timings over checkpointed jobs more accurately. There are some tests for the new timer added to the test directory.

I think there are multiple options for merging this. The first would be to merge only the changes to `GenomeAnalysisEngine`, leaving the old timer. This would potentially fix the issue for jobs with no runtime limit. The second would be to wholesale replace `SimpleTimer` with `CheckpointableTimer` - there shouldn't be a great deal of overhead, since it only does occasional checks of system time. In this case, both `Timer` and `SimpleTimer` could be dropped. Finally, we could add a command line switch to enable to new checkpointable timer. For this I might need advice from somebody who understands the argument parsing system and where the most appropriate place to add the option would be.