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/07/29 17:50:14 UTC
[GitHub] [hive] zabetak opened a new pull request #1331: HIVE-23946: Improve control flow and error handling in QTest dataset loading/unloading
zabetak opened a new pull request #1331:
URL: https://github.com/apache/hive/pull/1331
1. Remove redundant boolean return type from QTestDatasetHandler#initDataset and QTestDatasetHandler#unloadDataset (the method return true or fails).
2. Replace AssertionError with RuntimeException to allow callers handle failures properly (AssertionErrors are not meant to be caught).
3. Remove table to unload from src tables only after sucessfull unload (not before)
4. Reset 'missingTables' and 'tableToUnload' variables in case of failure on loading/unloading dataset, otherwise all subsequent tests using the same dataset handler will fail.
5. Turn 'missingTables' from class variable to instance variable; there is no reason to share this variable among instances; we need to have a way to know globally which tables are loaded but for this we have 'srcTables'.
----------------------------------------------------------------
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
[GitHub] [hive] github-actions[bot] closed pull request #1331: HIVE-23946: Improve control flow and error handling in QTest dataset loading/unloading
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #1331:
URL: https://github.com/apache/hive/pull/1331
----------------------------------------------------------------
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
[GitHub] [hive] zabetak commented on a change in pull request #1331: HIVE-23946: Improve control flow and error handling in QTest dataset loading/unloading
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #1331:
URL: https://github.com/apache/hive/pull/1331#discussion_r464505336
##########
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:
Given that there are no test failures I would say no but let's wait to see what @kgyrtkirk has to say :) On the other hand, as I noted in the JIRA there is code that does not work well if an assertion is thrown here.
----------------------------------------------------------------
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
[GitHub] [hive] zabetak commented on pull request #1331: HIVE-23946: Improve control flow and error handling in QTest dataset loading/unloading
Posted by GitBox <gi...@apache.org>.
zabetak commented on pull request #1331:
URL: https://github.com/apache/hive/pull/1331#issuecomment-667215984
Hey @kgyrtkirk , @abstractdog you seem to have touched this code recently. Can you please review?
----------------------------------------------------------------
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
[GitHub] [hive] abstractdog commented on a change in pull request #1331: HIVE-23946: Improve control flow and error handling in QTest dataset loading/unloading
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [hive] zabetak commented on a change in pull request #1331: HIVE-23946: Improve control flow and error handling in QTest dataset loading/unloading
Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #1331:
URL: https://github.com/apache/hive/pull/1331#discussion_r464503259
##########
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:
Indeed there is a check-then-act race condition here. I was hoping to fix this without making `missingTables` and `tablesToUnload` static but looking at the code that I committed it seems that I screwed up something while I was rebasing :D I will address this in the following commits (hopefully :crossed_fingers: ). Thanks for catching this!
----------------------------------------------------------------
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
[GitHub] [hive] github-actions[bot] commented on pull request #1331: HIVE-23946: Improve control flow and error handling in QTest dataset loading/unloading
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #1331:
URL: https://github.com/apache/hive/pull/1331#issuecomment-703183124
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Feel free to reach out on the dev@hive.apache.org list if the patch is in need of reviews.
----------------------------------------------------------------
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