review comments

broadinstitute · Apr 29, 2015 · 40babcf · 40babcf
1 parent e12e51b
commit 40babcf
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 31 deletions.
diff --git a/src/main/java/org/broadinstitute/hellbender/Main.java b/src/main/java/org/broadinstitute/hellbender/Main.java
@@ -45,12 +45,12 @@ public class Main {
     /**
      * exit value when an unrecoverable {@link UserException} occurs
      */
-    private static final int USER_EXCEPTION_EXIT_VALUE = 2;
+    public static final int USER_EXCEPTION_EXIT_VALUE = 2;
 
     /**
      * exit value when any unrecoverable exception other than {@link UserException} occurs
      */
-    private static final int ANY_OTHER_EXCEPTION_EXIT_VALUE = 1;
+    public static final int ANY_OTHER_EXCEPTION_EXIT_VALUE = 1;
 
     /**
      * The packages we wish to include in our command line *

diff --git a/...ain/java/org/broadinstitute/hellbender/tools/picard/illumina/ExtractIlluminaBarcodes.java b/...ain/java/org/broadinstitute/hellbender/tools/picard/illumina/ExtractIlluminaBarcodes.java
@@ -1,18 +1,21 @@
 package org.broadinstitute.hellbender.tools.picard.illumina;
 
+import htsjdk.samtools.SAMException;
 import htsjdk.samtools.metrics.MetricBase;
 import htsjdk.samtools.metrics.MetricsFile;
 import htsjdk.samtools.util.Log;
 import org.broadinstitute.hellbender.cmdline.Argument;
 import org.broadinstitute.hellbender.cmdline.CommandLineProgramProperties;
 import org.broadinstitute.hellbender.cmdline.PicardCommandLineProgram;
 import org.broadinstitute.hellbender.cmdline.programgroups.IlluminaProgramGroup;
+import org.broadinstitute.hellbender.exceptions.UserException;
 import org.broadinstitute.hellbender.tools.picard.illumina.parser.*;
 import org.broadinstitute.hellbender.tools.picard.illumina.parser.readers.BclQualityEvaluationStrategy;
 import org.broadinstitute.hellbender.utils.text.parsers.TabbedTextFileWithHeaderParser;
 
 import java.io.BufferedWriter;
 import java.io.File;
+import java.io.IOException;
 import java.text.NumberFormat;
 import java.util.*;
 
@@ -161,18 +164,22 @@ protected Object doWork() {
             );
             extractors.add(extractor);
         }
-        extractors.forEach(ExtractIlluminaBarcodes.PerTileBarcodeExtractor::run);
-
+        for (PerTileBarcodeExtractor  perTileBarcodeExtractor: extractors){
+            try {
+                perTileBarcodeExtractor.run();
+            } catch (IOException | SAMException e) {
+                //Note: HTSJDK wraps IOExceptions in SAMExceptions which causes us to have to catch both kinds and report as UserExceptions.
+                LOG.error("Abandoning metrics calculation because one or more PerTileBarcodeExtractors failed.");
+                throw new UserException("Abandoning metrics calculation because one or more PerTileBarcodeExtractors failed.", e);
+            }
+        }
         LOG.info("Processed " + extractors.size() + " tiles.");
+
         for (final PerTileBarcodeExtractor extractor : extractors) {
             for (final String key : barcodeToMetrics.keySet()) {
                 barcodeToMetrics.get(key).merge(extractor.getMetrics().get(key));
             }
             noMatchMetric.merge(extractor.getNoMatchMetric());
-            if (extractor.getException() != null) {
-                LOG.error("Abandoning metrics calculation because one or more PerTileBarcodeExtractors failed.");
-                return 4;
-            }
         }
 
         // Finish metrics tallying.
@@ -467,7 +474,6 @@ private static class PerTileBarcodeExtractor {
         private final File barcodeFile;
         private final Map<String, BarcodeMetric> metrics;
         private final BarcodeMetric noMatch;
-        private Exception exception = null;
         private final boolean usingQualityScores;
         private final IlluminaDataProvider provider;
         private final ReadStructure outputReadStructure;
@@ -528,14 +534,10 @@ public BarcodeMetric getNoMatchMetric() {
             return this.noMatch;
         }
 
-        public Exception getException() {
-            return this.exception;
-        }
-
         /**
          * run method which extracts barcodes and accumulates metrics for an entire tile
          */
-        public void run() {
+        public void run() throws IOException {
             try {
                 LOG.info("Extracting barcodes for tile " + tile);
 
@@ -561,14 +563,10 @@ public void run() {
                         for (final byte[] bc : barcodeSubsequences) {
                             writer.write(bytesToString(bc));
                         }
-                        writer.write("\t" + yOrN + "\t" + match.barcode + "\t" + valueOf(match.mismatches) +
-                                "\t" + valueOf(match.mismatchesToSecondBest));
+                        writer.write("\t" + yOrN + "\t" + match.barcode + "\t" + valueOf(match.mismatches) + "\t" + valueOf(match.mismatchesToSecondBest));
                         writer.newLine();
                     }
                 }
-            } catch (final Exception e) {
-                LOG.error(e, "Error processing tile ", this.tile);
-                this.exception = e;
             } finally {
                 provider.close();
             }

diff --git a/...java/org/broadinstitute/hellbender/tools/picard/illumina/ExtractIlluminaBarcodesTest.java b/...java/org/broadinstitute/hellbender/tools/picard/illumina/ExtractIlluminaBarcodesTest.java
@@ -1,33 +1,29 @@
 package org.broadinstitute.hellbender.tools.picard.illumina;
 
 import htsjdk.samtools.metrics.MetricsFile;
-import htsjdk.samtools.util.IOUtil;
 import org.broadinstitute.hellbender.CommandLineProgramTest;
+import org.broadinstitute.hellbender.Main;
+import org.broadinstitute.hellbender.exceptions.UserException;
 import org.broadinstitute.hellbender.tools.picard.illumina.parser.ClusterData;
 import org.broadinstitute.hellbender.tools.picard.illumina.parser.IlluminaDataProvider;
 import org.broadinstitute.hellbender.tools.picard.illumina.parser.IlluminaDataProviderFactory;
-import org.broadinstitute.hellbender.tools.picard.illumina.parser.IlluminaDataType;
 import org.broadinstitute.hellbender.tools.picard.illumina.parser.ReadStructure;
 import org.broadinstitute.hellbender.tools.picard.illumina.parser.readers.BclQualityEvaluationStrategy;
 import org.broadinstitute.hellbender.utils.text.parsers.BasicInputParser;
-import org.testng.Assert;
 import org.testng.annotations.AfterTest;
 import org.testng.annotations.BeforeTest;
 import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
 import java.io.File;
 import java.io.FileReader;
-import java.util.*;
+import java.util.ArrayList;
+import java.util.List;
 
-import static htsjdk.samtools.util.IOUtil.copyDirectoryTree;
-import static htsjdk.samtools.util.IOUtil.deleteDirectoryTree;
-import static htsjdk.samtools.util.IOUtil.getFilesMatchingRegexp;
+import static htsjdk.samtools.util.IOUtil.*;
 import static java.util.Arrays.asList;
 import static java.util.Arrays.sort;
-import static org.broadinstitute.hellbender.tools.picard.illumina.parser.IlluminaDataType.Barcodes;
-import static org.broadinstitute.hellbender.tools.picard.illumina.parser.IlluminaDataType.BaseCalls;
-import static org.broadinstitute.hellbender.tools.picard.illumina.parser.IlluminaDataType.QualityScores;
+import static org.broadinstitute.hellbender.tools.picard.illumina.parser.IlluminaDataType.*;
 import static org.broadinstitute.hellbender.tools.picard.illumina.parser.readers.BclQualityEvaluationStrategy.ILLUMINA_ALLEGED_MINIMUM_QUALITY;
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertTrue;
@@ -109,7 +105,7 @@ public void testPairedEndWithBarcodeOnSecondEnd() throws Exception {
         assertEquals(metricsFile.getMetrics().get(12).PERFECT_MATCHES, 1);
     }
 
-    @Test
+    @Test(expectedExceptions = UserException.class)
     public void testNonWritableOutputFile() throws Exception {
         final File existingFile = new File(basecallsDir, "s_1_1101_barcode.txt.gz");
         try {
@@ -131,7 +127,7 @@ public void testNonWritableOutputFile() throws Exception {
                 args.add("--BARCODE");
                 args.add(barcode);
             }
-            assertEquals(runCommandLine(args), 4);
+            assertEquals(runCommandLine(args), Main.ANY_OTHER_EXCEPTION_EXIT_VALUE);
         } finally {
             existingFile.setWritable(true);
         }