[
https://issues.apache.org/jira/browse/SENTRY-1377?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Vadim Spector updated SENTRY-1377:
----------------------------------
Description:
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. Initialization retry logic uses recursion, 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.
was:
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. Initialization retry logic uses recursion, as in startHiveServer2(),
verifyQuery(), verifyOnAllSubDirs. Better to implement it via straightforward
retry loop.
> 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
> Reporter: Vadim Spector
> Priority: Minor
>
> 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. Initialization retry logic uses recursion, 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.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
