Add oracle live ITs [Batch 1] - #4272
dhwanilpatel wants to merge 1 commit into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the first batch of integration tests for the DataStream to Spanner template, specifically targeting Oracle source databases. These tests ensure robust handling of various Oracle data types, schema overrides, and complex scenarios like reserved keywords and timezone conversions. Additionally, the PR updates the CI/CD infrastructure to support necessary database authentication and provides improved utilities for managing Oracle resources during test execution. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds comprehensive integration tests and resource management support for Oracle database migrations to Spanner using Datastream. It introduces several new integration test classes covering DDL, datatype conversions, string and file overrides, reserved keywords, timezone handling, and session configurations, alongside a shared Oracle instance manager and a custom resource manager. The feedback suggests addressing a few critical issues: fixing the cleanup order in SpannerOracleResourceManager to prevent executing queries on a closed connection pool, removing an unused empty method, avoiding modifying the global JVM timezone in tests, using standard JDBC execution instead of fragile manual SQL parsing, ensuring consistent default passwords, and properly restoring the thread interrupted status when catching InterruptedException across multiple test files.
| public void cleanupAll() { | ||
| super.cleanupAll(); | ||
|
|
||
| String userToDrop = this.getUsername(); | ||
| if (userToDrop != null | ||
| && !userToDrop.equalsIgnoreCase("system") | ||
| && !userToDrop.contains("sysdba")) { | ||
| LOG.info("Attempting to dynamically drop isolated Oracle user: {}", userToDrop); | ||
| try { | ||
| if (userToDrop.toUpperCase().startsWith("C##")) { | ||
| SharedOracleLiveITInstance.getInstance() | ||
| .runSQLUpdate("DROP USER " + userToDrop + " CASCADE"); | ||
| } else { | ||
| SharedOracleLiveITInstance.getInstance() | ||
| .runSQLUpdate("DROP USER " + userToDrop + " CASCADE"); | ||
| } | ||
| LOG.info("Successfully dropped Oracle schema: {}", userToDrop); | ||
| } catch (Exception e) { | ||
| LOG.warn("Failed to drop schema: " + userToDrop, e); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There are multiple issues with the current implementation of cleanupAll():
super.cleanupAll()is called first, which closes the connection pool ofthisinstance. Ifthisis the same instance returned bySharedOracleLiveITInstance.getInstance(), then callingrunSQLUpdateon it will fail because the pool is already closed.- The
if-elseblock on lines 54-60 has identical branches.
We can simplify and fix this by dropping the user before calling super.cleanupAll(), using this.runSQLUpdate(...) directly.
@Override
public void cleanupAll() {
String userToDrop = this.getUsername();
if (userToDrop != null
&& !userToDrop.equalsIgnoreCase("system")
&& !userToDrop.contains("sysdba")) {
LOG.info("Attempting to dynamically drop isolated Oracle user: {}", userToDrop);
try {
this.runSQLUpdate("DROP USER " + userToDrop + " CASCADE");
LOG.info("Successfully dropped Oracle schema: {}", userToDrop);
} catch (Exception e) {
LOG.warn("Failed to drop schema: " + userToDrop, e);
}
}
super.cleanupAll();
}| @Before | ||
| public void setUp() throws Exception { | ||
| java.util.TimeZone.setDefault(java.util.TimeZone.getTimeZone("UTC")); | ||
| skipBaseCleanup = true; |
There was a problem hiding this comment.
Modifying the global JVM default timezone via TimeZone.setDefault is a known anti-pattern in shared JVM test execution. It can cause flakiness or unexpected failures in other timezone-sensitive tests running in the same JVM. If absolutely necessary, please store the original timezone in a static field and restore it in the @AfterClass cleanup method, or configure the timezone explicitly on the timezone-sensitive components.
| if (stmt.toLowerCase().trim().startsWith("select")) { | ||
| statement.executeQuery(stmt); | ||
| } else { | ||
| statement.executeUpdate(stmt); | ||
| } |
There was a problem hiding this comment.
Instead of manually parsing the SQL string to check if it starts with "select" (which can be fragile if there are comments or leading whitespace), you can use the standard JDBC statement.execute(stmt) method. It automatically handles both query and update/DDL statements.
statement.execute(stmt);| String url = "jdbc:oracle:thin:@//" + System.getProperty("cloudOracleHost") + ":1521/XE"; | ||
| String user = System.getProperty("cloudOracleUsername", "system"); | ||
| String pass = System.getProperty("cloudOraclePassword", "Test@Password123"); |
There was a problem hiding this comment.
The default password here ("Test@Password123") is inconsistent with the default password used elsewhere in this class ("TestPassword123" on line 36). Additionally, cloudOracleHost should default to "localhost" to prevent a null host in the JDBC URL if the system property is not set.
| String url = "jdbc:oracle:thin:@//" + System.getProperty("cloudOracleHost") + ":1521/XE"; | |
| String user = System.getProperty("cloudOracleUsername", "system"); | |
| String pass = System.getProperty("cloudOraclePassword", "Test@Password123"); | |
| String url = "jdbc:oracle:thin:@//" + System.getProperty("cloudOracleHost", "localhost") + ":1521/XE"; | |
| String user = System.getProperty("cloudOracleUsername", "system"); | |
| String pass = System.getProperty("cloudOraclePassword", "TestPassword123"); |
| getJDBCPrefix(), this.getHost(), this.getPort(getJDBCPort()), this.getDatabaseName()); | ||
| } | ||
|
|
||
| public void runSQLUpdate(String sql, String user) {} |
There was a problem hiding this comment.
This method is empty and is not used anywhere in the codebase. It should be removed to keep the code clean.
| try { | ||
| Thread.sleep(CUTOVER_MILLIS); | ||
| } catch (InterruptedException e) { | ||
| } |
There was a problem hiding this comment.
When catching InterruptedException, you should restore the interrupted status of the thread by calling Thread.currentThread().interrupt(). Ignoring it can prevent thread pools or frameworks from correctly managing the thread's lifecycle.
| try { | |
| Thread.sleep(CUTOVER_MILLIS); | |
| } catch (InterruptedException e) { | |
| } | |
| try { | |
| Thread.sleep(CUTOVER_MILLIS); | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| } |
| try { | ||
| Thread.sleep(CUTOVER_MILLIS); | ||
| } catch (InterruptedException e) { | ||
| } |
There was a problem hiding this comment.
When catching InterruptedException, you should restore the interrupted status of the thread by calling Thread.currentThread().interrupt(). Ignoring it can prevent thread pools or frameworks from correctly managing the thread's lifecycle.
| try { | |
| Thread.sleep(CUTOVER_MILLIS); | |
| } catch (InterruptedException e) { | |
| } | |
| try { | |
| Thread.sleep(CUTOVER_MILLIS); | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| } |
| try { | ||
| Thread.sleep(CUTOVER_MILLIS); | ||
| } catch (InterruptedException e) { | ||
| } |
There was a problem hiding this comment.
When catching InterruptedException, you should restore the interrupted status of the thread by calling Thread.currentThread().interrupt(). Ignoring it can prevent thread pools or frameworks from correctly managing the thread's lifecycle.
| try { | |
| Thread.sleep(CUTOVER_MILLIS); | |
| } catch (InterruptedException e) { | |
| } | |
| try { | |
| Thread.sleep(CUTOVER_MILLIS); | |
| } catch (InterruptedException e) { | |
| Thread.currentThread().interrupt(); | |
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4272 +/- ##
============================================
+ Coverage 56.35% 62.50% +6.14%
+ Complexity 7858 3671 -4187
============================================
Files 1154 594 -560
Lines 73194 35945 -37249
Branches 8580 4033 -4547
============================================
- Hits 41252 22467 -18785
+ Misses 29158 12330 -16828
+ Partials 2784 1148 -1636
🚀 New features to boost your workflow:
|
1 participant
Adding First batch of Oracle Live template ITs.
List of ITs: