[DISQ-29] Use a separate instance of VCFCodec for each partition #30

tomwhite · 2018-09-10T10:39:26Z

This exposes the bug in #29, by using all available cores on a machine for testing (in BaseTest). (I was also able to repoduce the bug when testing on large files.)

The fix relies on the latest htsjdk release.

…ition.

heuermh · 2018-09-10T14:40:31Z

src/main/java/org/disq_bio/disq/impl/formats/bam/HeaderlessBamOutputFormat.java

@@ -31,7 +32,7 @@

    public BamRecordWriter(Configuration conf, Path file, SAMFileHeader header) throws IOException {
      this.out = file.getFileSystem(conf).create(file);
-      BlockCompressedOutputStream compressedOut = new BlockCompressedOutputStream(out, null);
+      BlockCompressedOutputStream compressedOut = new BlockCompressedOutputStream(out, (File) null);


I hate htsjdk.

Anyone object to making an assertion in this library that all method parameters should be nonnull by default?

@ParametersAreNonnullByDefault package foo; import javax.annotation.ParametersAreNonnullByDefault;

https://github.com/google/guava/wiki/UsingAndAvoidingNullExplained#using-and-avoiding-null

@heuermh I'm not very familiar with the various nullness annotations. What does adding that do? Is it just a statement of principle or is enforced somehow?

As far as I know, a statement of principle, although there may be tools that use that annotation in static code analysis.

Throughout the guava library method parameters are checked via Preconditions to fail fast on nulls. Adding a dependency on guava comes with its own set of problems though.

I'd be happy to simply say we're not going to design our API to accept nulls as method parameters.

+1 to not allowing nulls in the API. I'd rather not have a Guava dependency though.

+1 also for non-null (I am trying the same in htsjdk-next). Maybe it is worth to check the Bean Validation project for constraints in the library (I am planning to propose this also in htsjdk-next).

lbergelson · 2018-09-10T15:29:34Z

src/main/java/org/disq_bio/disq/impl/formats/vcf/VcfSource.java

@@ -96,14 +97,18 @@ private static InputStream bufferAndDecompressIfNecessary(final InputStream in)
    }
    enableBGZFCodecs(conf);

-    Broadcast<VCFCodec> vcfCodecBroadcast = jsc.broadcast(getVCFCodec(jsc, path));
+    VCFCodec vcfCodec = getVCFCodec(jsc, path);
+    VCFHeader header = vcfCodec.getHeader();


Could you add a comment explaining why this is necessary for future generations? The version thing is very non obvious.

magicDGS

Just some comments

magicDGS · 2018-09-10T18:20:59Z

src/main/java/org/disq_bio/disq/impl/formats/bam/HeaderlessBamOutputFormat.java

@@ -31,7 +32,7 @@

    public BamRecordWriter(Configuration conf, Path file, SAMFileHeader header) throws IOException {
      this.out = file.getFileSystem(conf).create(file);
-      BlockCompressedOutputStream compressedOut = new BlockCompressedOutputStream(out, null);
+      BlockCompressedOutputStream compressedOut = new BlockCompressedOutputStream(out, (File) null);


+1 also for non-null (I am trying the same in htsjdk-next). Maybe it is worth to check the Bean Validation project for constraints in the library (I am planning to propose this also in htsjdk-next).

magicDGS · 2018-09-10T18:22:36Z

src/main/java/org/disq_bio/disq/impl/formats/vcf/VcfOutputFormat.java

@@ -33,7 +34,7 @@ public VcfRecordWriter(Configuration conf, Path file, VCFHeader header, String e
      boolean compressed =
          extension.endsWith(BGZFCodec.DEFAULT_EXTENSION) || extension.endsWith(".gz");
      if (compressed) {
-        out = new BlockCompressedOutputStream(out, null);
+        out = new BlockCompressedOutputStream(out, (File) null);


How much I miss in htsjdk a method to set the filename/source (as a String) to bgzip streams...

magicDGS · 2018-09-10T18:23:08Z

src/main/java/org/disq_bio/disq/impl/formats/vcf/VcfSource.java

@@ -96,14 +97,20 @@ private static InputStream bufferAndDecompressIfNecessary(final InputStream in)
    }
    enableBGZFCodecs(conf);

-    Broadcast<VCFCodec> vcfCodecBroadcast = jsc.broadcast(getVCFCodec(jsc, path));
+    VCFCodec vcfCodec = getVCFCodec(jsc, path);


Can be final (also in other parts of the code)

magicDGS · 2018-09-10T18:23:47Z

src/test/java/org/disq_bio/disq/BaseTest.java

@@ -20,7 +20,7 @@
  public static void setup() {
    SparkConf sparkConf = new SparkConf();
    sparkConf.set("spark.serializer", "org.apache.spark.serializer.KryoSerializer");
-    jsc = new JavaSparkContext("local", "myapp", sparkConf);
+    jsc = new JavaSparkContext("local[*]", "myapp", sparkConf);


Could it be possible to have both kind of tests?

It would need some more work. I think it's most useful to test with multiple cores as it's most likely to expose concurrency issues (as it did in this case).

But having consistency between both versions might be also beneficial, right? Maybe we can open an issue if someone wants to look at refactoring to test both...

tomwhite · 2018-09-17T13:55:27Z

Are we happy for this to go in now?

magicDGS · 2018-09-17T17:35:11Z

👍

tomwhite · 2018-09-18T08:22:31Z

Thanks @magicDGS!

lbergelson · 2018-11-02T17:54:11Z

Woops, was assigning the wrong ticket..

tomwhite added 3 commits September 10, 2018 10:28

Upgrade to htsjdk 2.16.1

d8a8d0c

Use all available cores for testing

9a4eef8

VCFCodec is not thread-safe, so use a separate instance for each part…

8f6dfc1

…ition.

tomwhite mentioned this pull request Sep 10, 2018

HtsjdkVariantsRdd fails when reading with concurrent tasks #29

Closed

heuermh reviewed Sep 10, 2018

View reviewed changes

lbergelson reviewed Sep 10, 2018

View reviewed changes

Add comments

a62a09f

magicDGS reviewed Sep 10, 2018

View reviewed changes

Make VCFCodec final

d3370a1

tomwhite force-pushed the vcf-concurrency-fix branch from 7556608 to d3370a1 Compare September 14, 2018 13:47

tomwhite merged commit b612485 into disq-bio:master Sep 18, 2018

tomwhite deleted the vcf-concurrency-fix branch September 18, 2018 08:22

magicDGS mentioned this pull request Sep 22, 2018

Consider to test with single in addition to multiple cores #37

Open

heuermh added this to the 0.1.0 milestone Oct 24, 2018

lbergelson assigned tomwhite and unassigned tomwhite Nov 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DISQ-29] Use a separate instance of VCFCodec for each partition #30

[DISQ-29] Use a separate instance of VCFCodec for each partition #30

tomwhite commented Sep 10, 2018

heuermh Sep 10, 2018

lbergelson Sep 10, 2018

heuermh Sep 10, 2018

tomwhite Sep 10, 2018

magicDGS Sep 10, 2018

lbergelson Sep 10, 2018

tomwhite Sep 10, 2018

magicDGS left a comment

magicDGS Sep 10, 2018

magicDGS Sep 10, 2018

magicDGS Sep 10, 2018

tomwhite Sep 13, 2018

magicDGS Sep 10, 2018

tomwhite Sep 13, 2018

magicDGS Sep 17, 2018

tomwhite commented Sep 17, 2018

magicDGS commented Sep 17, 2018

tomwhite commented Sep 18, 2018

lbergelson commented Nov 2, 2018

[DISQ-29] Use a separate instance of VCFCodec for each partition #30

[DISQ-29] Use a separate instance of VCFCodec for each partition #30

Conversation

tomwhite commented Sep 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magicDGS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomwhite commented Sep 17, 2018

magicDGS commented Sep 17, 2018

tomwhite commented Sep 18, 2018

lbergelson commented Nov 2, 2018