TEZ-2814. ATSImportTool has a return statement in a finally block (rb…

…alamohan)
Poneyo · Oct 29, 2015 · dcc5108 · dcc5108
1 parent 19808b7
commit dcc5108
Show file tree

Hide file tree

Showing 3 changed files with 15 additions and 22 deletions.
diff --git a/CHANGES.txt b/CHANGES.txt
@@ -7,6 +7,7 @@ INCOMPATIBLE CHANGES
   TEZ-2679. Admin forms of launch env settings
 
 ALL CHANGES:
+  TEZ-2814. ATSImportTool has a return statement in a finally block
   TEZ-2906. Compilation fails with hadoop 2.2.0
   TEZ-2900. Ignore V_INPUT_DATA_INFORMATION when vertex is in Failed/Killed/Error
   TEZ-2244. PipelinedSorter: Progressive allocation for sort-buffers

diff --git a/tez-plugins/tez-history-parser/src/main/java/org/apache/tez/history/ATSImportTool.java b/tez-plugins/tez-history-parser/src/main/java/org/apache/tez/history/ATSImportTool.java
@@ -121,13 +121,11 @@ public class ATSImportTool extends Configured implements Tool {
   private final String baseUri;
   private final String dagId;
 
-  private final File downloadDir;
   private final File zipFile;
   private final Client httpClient;
   private final TezDAGID tezDAGID;
 
-  public ATSImportTool(String baseUri, String dagId, File downloadDir, int batchSize)
-      throws TezException {
+  public ATSImportTool(String baseUri, String dagId, File downloadDir, int batchSize) {
     Preconditions.checkArgument(!Strings.isNullOrEmpty(dagId), "dagId can not be null or empty");
     Preconditions.checkArgument(downloadDir != null, "downloadDir can not be null");
     tezDAGID = TezDAGID.fromString(dagId);
@@ -138,7 +136,6 @@ public ATSImportTool(String baseUri, String dagId, File downloadDir, int batchSi
 
     this.httpClient = getHttpClient();
 
-    this.downloadDir = downloadDir;
     this.zipFile = new File(downloadDir, this.dagId + ".zip");
 
     boolean result = downloadDir.mkdirs();
@@ -268,8 +265,7 @@ private void downloadJSONArrayFromATS(String url, ZipOutputStream zos, String ta
     }
   }
 
-  private String logErrorMessage(ClientResponse response) throws IOException {
-    StringBuilder sb = new StringBuilder();
+  private void logErrorMessage(ClientResponse response) throws IOException {
     LOG.error("Response status={}", response.getClientResponseStatus().toString());
     LineIterator it = null;
     try {
@@ -283,7 +279,6 @@ private String logErrorMessage(ClientResponse response) throws IOException {
         it.close();
       }
     }
-    return sb.toString();
   }
 
   //For secure cluster, this should work as long as valid ticket is available in the node.
@@ -433,9 +428,8 @@ static String getBaseTimelineURL(String yarnTimelineAddress, Configuration conf)
   }
 
   @VisibleForTesting
-  public static int process(String[] args) {
+  public static int process(String[] args) throws Exception {
     Options options = buildOptions();
-    int result = -1;
     try {
       Configuration conf = new Configuration();
       CommandLine cmdLine = new GnuParser().parse(options, args);
@@ -449,21 +443,15 @@ public static int process(String[] args) {
       int batchSize = (cmdLine.hasOption(BATCH_SIZE)) ?
           (Integer.parseInt(cmdLine.getOptionValue(BATCH_SIZE))) : BATCH_SIZE_DEFAULT;
 
-      result = ToolRunner.run(conf, new ATSImportTool(baseTimelineURL, dagId,
+      return ToolRunner.run(conf, new ATSImportTool(baseTimelineURL, dagId,
           downloadDir, batchSize), args);
-
-      return result;
-    } catch (MissingOptionException missingOptionException) {
-      LOG.error("Error in parsing options ", missingOptionException);
-      printHelp(options);
     } catch (ParseException e) {
       LOG.error("Error in parsing options ", e);
       printHelp(options);
+      throw e;
     } catch (Exception e) {
       LOG.error("Error in processing ", e);
       throw e;
-    } finally {
-      return result;
     }
   }
 

diff --git a/tez-plugins/tez-history-parser/src/test/java/org/apache/tez/history/TestHistoryParser.java b/tez-plugins/tez-history-parser/src/test/java/org/apache/tez/history/TestHistoryParser.java
@@ -19,6 +19,8 @@
 package org.apache.tez.history;
 
 import com.google.common.collect.Sets;
+import com.sun.tools.internal.ws.processor.ProcessorException;
+import org.apache.commons.cli.ParseException;
 import org.apache.commons.io.FileUtils;
 import org.apache.commons.lang.RandomStringUtils;
 import org.apache.hadoop.conf.Configuration;
@@ -92,9 +94,7 @@
 import java.util.List;
 import java.util.Map;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.*;
 
 public class TestHistoryParser {
 
@@ -342,8 +342,12 @@ public void testParserWithSuccessfulJob_InvalidATS() throws Exception {
         atsAddress
       };
 
-    int result = ATSImportTool.process(args);
-    assertTrue(result == -1);
+    try {
+      int result = ATSImportTool.process(args);
+      fail("Should have failed with processException");
+    } catch(ParseException e) {
+      //expects exception
+    }
   }
 
   /**