You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2020/08/03 12:37:01 UTC
[GitHub] [hive] abstractdog commented on a change in pull request #1331: HIVE-23946: Improve control flow and error handling in QTest dataset loading/unloading
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
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org