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

split async samtools and tribble options #529

Merged
merged 1 commit into from
Mar 22, 2016
Merged

Conversation

akiezun
Copy link
Contributor

@akiezun akiezun commented Mar 21, 2016

Description

This PR takes over from #528 and splits USE_ASYNC_IO into 2 options, USE_ASYNC_IO_FOR_SAMTOOLS and USE_ASYNC_IO_FOR_TRIBBLE because clients like GATK want more fine grained controls and also the performance characteristics of samtools vs tribble are very different (see #528) for examples and timings.

@droazen can you review? @yfarjoun FYI

@@ -19,9 +19,18 @@
/** Should MD5 files be created when writing out SAM and BAM files? Default = false. */
public static final boolean CREATE_MD5;

/** Should asynchronous I/O be used when writing out SAM and BAM files (one thread per file). Default = false. */
/** Should asynchronous I/O be used when writing out SAM, BAM, FASTQ files (one thread per file). Default = false. */
@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be deprecated. The semantics of this option are actually unchanged -- differences only arise if you opt-in to use of one of the two new options below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, you should mention explicitly in the docs for both this option and the others below that this option takes precedence over the other two.

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

@droazen
Copy link
Contributor

droazen commented Mar 21, 2016

Review complete -- back to @akiezun

@droazen droazen assigned akiezun and unassigned droazen Mar 21, 2016
@akiezun
Copy link
Contributor Author

akiezun commented Mar 22, 2016

back to @droazen

@akiezun akiezun assigned droazen and unassigned akiezun Mar 22, 2016
@@ -19,9 +22,26 @@
/** Should MD5 files be created when writing out SAM and BAM files? Default = false. */
public static final boolean CREATE_MD5;

/** Should asynchronous I/O be used when writing out SAM and BAM files (one thread per file). Default = false. */
/** Should asynchronous I/O be used where supported in tribble/samtools/etc. (one thread per file).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"in tribble/samtools/etc." -> "throughout all of htsjdk"

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

@droazen
Copy link
Contributor

droazen commented Mar 22, 2016

Second-pass review complete -- back to @akiezun. Merge after addressing comments.

akiezun added a commit that referenced this pull request Mar 22, 2016
split async samtools and tribble options
@akiezun akiezun merged commit 9574848 into master Mar 22, 2016
@akiezun akiezun deleted the ak_asynch_IO_Options branch March 22, 2016 17:35
This pull request was closed.
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