[jira] [Commented] (SENTRY-1377) improve handling of failures, both in tests and after-test cleanup, in TestHDFSIntegration.java

["Hadoop QA (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/SENTRY-1377?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15368824#comment-15368824 ] 

Hadoop QA commented on SENTRY-1377:
-----------------------------------

Here are the results of testing the latest attachment
https://issues.apache.org/jira/secure/attachment/12816938/SENTRY-1377.001.patch against master.

{color:green}Overall:{color} +1 all checks pass

{color:green}SUCCESS:{color} all tests passed

Console output: https://builds.apache.org/job/PreCommit-SENTRY-Build/1850/console

This message is automatically generated.

> improve handling of failures, both in tests and after-test cleanup, in TestHDFSIntegration.java
> -----------------------------------------------------------------------------------------------
>
>                 Key: SENTRY-1377
>                 URL: https://issues.apache.org/jira/browse/SENTRY-1377
>             Project: Sentry
>          Issue Type: Test
>          Components: Sentry
>    Affects Versions: 1.8.0
>            Reporter: Vadim Spector
>            Priority: Minor
>             Fix For: 1.8.0
>
>         Attachments: SENTRY-1377.001.patch
>
>
> 1. TestHDFSIntegration.java should provide best-attempt cleanup in cleanAfterTest() method. Currently, if any cleanup operation fails, the rest of cleanup code is not executed. Cleanup logic would not normally fail if corresponding test succeeds. But if some do not, short-circuiting some cleanup logic tends to cascade secondary failures which complicate troubleshooting.
> 2. Test methods would short-circuit closing database Connection's and Statement's if the test logic fails. Moreover, some test methods have multiple initializations of Connection ans Statement, but only one stmt.close(); conn.close(); pair at the end. Correct pattern for each distinct {Connection,Statement} pair would be:
> conn = null;
> stmt = null;
> try {
> ..... test logic here, including conn and stmt initialization ...
> } finally {
>   safeClose(conn, stmt);
> }
> where
> private static safeClose(Connection conn, Statement stmt) {
>   if (stmt != null) try { stmt.close(); } catch (Exception ignore) {}
>   if (conn != null) try { conn.close(); } catch (Exception ignore) {}
> }
> Testing jdbc driver implementation is not in the scope of HDFS integration tests, so ignoring Connection.close() and Statement.close() failures should not be a concern for this test class.
> 3. Retry logic uses recursion in some places, as in startHiveServer2(), verifyQuery(), verifyOnAllSubDirs. Better to implement it via straightforward retry loop. Exception stack trace is more confusing than it needs to be in case of recursive calls. Plus, with NUM_RETRIES == 10, at least theoretically, it creates running out of stack as an unnecessary failure mechanism.
> 4. startHiveServer2() ignores hiveServer2.start() call failure, only logging info message.
> 5. Starting hiveServer2 and Hive metastore in separate threads and then keeping those threads alive seems unnecessary, since both servers' start() methods create servers running on their own threads anyway. It effectively leads to ignoring the start() method failure for both servers.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)