hive-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (Jira)" <j...@apache.org>
Subject [jira] [Work logged] (HIVE-23946) Improve control flow and error handling in QTest dataset loading/unloading
Date Mon, 03 Aug 2020 12:38:00 GMT

     [ https://issues.apache.org/jira/browse/HIVE-23946?focusedWorklogId=465664&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-465664
]

ASF GitHub Bot logged work on HIVE-23946:
-----------------------------------------

                Author: ASF GitHub Bot
            Created on: 03/Aug/20 12:37
            Start Date: 03/Aug/20 12:37
    Worklog Time Spent: 10m 
      Work Description: abstractdog commented on a change in pull request #1331:
URL: https://github.com/apache/hive/pull/1331#discussion_r464382232



##########
File path: itests/util/src/main/java/org/apache/hadoop/hive/ql/dataset/QTestDatasetHandler.java
##########
@@ -52,8 +51,8 @@
 
   private File datasetDir;
   private static Set<String> srcTables;
-  private static Set<String> missingTables = new HashSet<>();

Review comment:
       @zabetak : I really want to have this non-static as you did, but I needed to change
it in HIVE-22617 as it caused flakyness in TestMTQueries (you can take a look at comments)...however,
I'm not really sure if parallel qtest running will be implemented properly in the near future,
so TestMTQueries is not a useful unit test

##########
File path: itests/util/src/main/java/org/apache/hadoop/hive/ql/dataset/QTestDatasetHandler.java
##########
@@ -84,23 +83,25 @@ public boolean initDataset(String table, CliDriver cliDriver) throws Exception
{
 
     try {
       CommandProcessorResponse result = cliDriver.processLine(commands);
-      LOG.info("Result from cliDrriver.processLine in initFromDatasets=" + result);
+      LOG.info("Result from cliDrriver.processLine in initDataset=" + result);
     } catch (CommandProcessorException e) {
-      Assert.fail("Failed during initFromDatasets processLine with code=" + e);
+      throw new RuntimeException("Failed while loading table " + table, e);
     }
-
-    return true;
+    // Add the talbe in sources if it is loaded sucessfully
+    addSrcTable(table);
   }
 
-  public boolean unloadDataset(String table, CliDriver cliDriver) throws Exception {
+  private void unloadDataset(String table, CliDriver cliDriver) {
     try {
+      // Remove table from sources otherwise the following command will fail due to EnforceReadOnlyTables.
+      removeSrcTable(table);
       CommandProcessorResponse result = cliDriver.processLine("drop table " + table);
-      LOG.info("Result from cliDrriver.processLine in initFromDatasets=" + result);
+      LOG.info("Result from cliDrriver.processLine in unloadDataset=" + result);
     } catch (CommandProcessorException e) {
-      Assert.fail("Failed during initFromDatasets processLine with code=" + e);
+      // If the unloading fails for any reason then add again the table to sources since
it is still there.
+      addSrcTable(table);
+      throw new RuntimeException("Failed while unloading table " + table, e);

Review comment:
       I'm fine with this change, but I don't know what was the original purpose... @kgyrtkirk
is there some place in the code which relies on assertion failures?

##########
File path: itests/util/src/main/java/org/apache/hadoop/hive/ql/dataset/QTestDatasetHandler.java
##########
@@ -84,23 +83,25 @@ public boolean initDataset(String table, CliDriver cliDriver) throws Exception
{
 
     try {
       CommandProcessorResponse result = cliDriver.processLine(commands);
-      LOG.info("Result from cliDrriver.processLine in initFromDatasets=" + result);
+      LOG.info("Result from cliDrriver.processLine in initDataset=" + result);
     } catch (CommandProcessorException e) {
-      Assert.fail("Failed during initFromDatasets processLine with code=" + e);
+      throw new RuntimeException("Failed while loading table " + table, e);
     }
-
-    return true;
+    // Add the talbe in sources if it is loaded sucessfully

Review comment:
       minor typo: "talbe"




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Issue Time Tracking
-------------------

    Worklog Id:     (was: 465664)
    Time Spent: 0.5h  (was: 20m)

> Improve control flow and error handling in QTest dataset loading/unloading
> --------------------------------------------------------------------------
>
>                 Key: HIVE-23946
>                 URL: https://issues.apache.org/jira/browse/HIVE-23946
>             Project: Hive
>          Issue Type: Improvement
>            Reporter: Stamatis Zampetakis
>            Assignee: Stamatis Zampetakis
>            Priority: Major
>              Labels: pull-request-available
>          Time Spent: 0.5h
>  Remaining Estimate: 0h
>
> This issue focuses mainly on the following methods:
> [QTestDatasetHandler#initDataset| https://github.com/apache/hive/blob/6fbd54c0af60276d49b237defb550938c9c32610/itests/util/src/main/java/org/apache/hadoop/hive/ql/dataset/QTestDatasetHandler.java#L76]
> [QTestDatasetHandler#unloadDataset|https://github.com/apache/hive/blob/6fbd54c0af60276d49b237defb550938c9c32610/itests/util/src/main/java/org/apache/hadoop/hive/ql/dataset/QTestDatasetHandler.java#L95]
> related to QTest dataset loading and unloading.
> The boolean return type in these methods is redundant since they either fail or return
true (they never return false).
> The methods should throw an Exception instead of an AssertionError to indicate failure.
This allows code higher up the stack to perform proper recovery and properly report the failure.
At the moment, if an AssertionError is raised from these methods dependent code (eg., [CoreCliDriver|https://github.com/apache/hive/blob/6fbd54c0af60276d49b237defb550938c9c32610/itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreCliDriver.java#L188])
fails to notice that the query has failed. 
> In case of failure in loading/unloading the environment (instance and class variables)
is not properly cleaned leading to failures in all subsequent tests.



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Mime
View raw message