Add oracle live ITs - #4172
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 significantly expands the integration testing coverage for Oracle-to-Spanner data migration templates. It introduces numerous new integration test classes to validate various scenarios, including DDL handling, DLQ retry logic, and sharded migrations. The changes also include necessary infrastructure updates to support Oracle database configuration in test environments and a fix for classloading in custom transformations. 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 a comprehensive suite of integration tests for the Datastream-to-Spanner template with an Oracle source, covering data types, sharding, reserved keywords, overrides, and dead-letter queue (DLQ) retry behaviors. It also updates the custom transformation class loader to use the current thread's context class loader. The review feedback highlights critical issues in the newly added tests that must be addressed before merging: several tests contain hardcoded absolute local file paths that will break CI/CD pipelines; multiple database resource managers are instantiated but never registered for cleanup, causing connection leaks; and a condition check lacks execution flags, which will trigger duplicate SQL inserts and unique constraint violations during retry loops.
| private String getCustomShardJarPath() { | ||
| return "/home/dhwanilpatel_google_com/MyStorage/OracleSupport/DataflowTemplates/v2/spanner-custom-shard/target/spanner-custom-shard-1.0-SNAPSHOT.jar"; | ||
| } |
There was a problem hiding this comment.
The custom shard jar path is hardcoded to an absolute local path specific to a developer's machine (/home/dhwanilpatel_google_com/...). This will cause the integration test to fail in CI/CD environments or on other developers' machines. Use a system property with a relative path fallback instead.
| private String getCustomShardJarPath() { | |
| return "/home/dhwanilpatel_google_com/MyStorage/OracleSupport/DataflowTemplates/v2/spanner-custom-shard/target/spanner-custom-shard-1.0-SNAPSHOT.jar"; | |
| } | |
| private String getCustomShardJarPath() { | |
| return System.getProperty("customShardJarPath", "../spanner-custom-shard/target/spanner-custom-shard-1.0-SNAPSHOT.jar"); | |
| } |
| private String getCustomShardJarPath() { | ||
| return "/home/dhwanilpatel_google_com/MyStorage/OracleSupport/DataflowTemplates/v2/spanner-custom-shard/target/spanner-custom-shard-1.0-SNAPSHOT.jar"; | ||
| } |
There was a problem hiding this comment.
The custom shard jar path is hardcoded to an absolute local path specific to a developer's machine. This will cause the integration test to fail in CI/CD environments or on other developers' machines. Use a system property with a relative path fallback instead.
| private String getCustomShardJarPath() { | |
| return "/home/dhwanilpatel_google_com/MyStorage/OracleSupport/DataflowTemplates/v2/spanner-custom-shard/target/spanner-custom-shard-1.0-SNAPSHOT.jar"; | |
| } | |
| private String getCustomShardJarPath() { | |
| return System.getProperty("customShardJarPath", "../spanner-custom-shard/target/spanner-custom-shard-1.0-SNAPSHOT.jar"); | |
| } |
| private String getCustomShardJarPath() { | ||
| return "/home/dhwanilpatel_google_com/MyStorage/OracleSupport/DataflowTemplates/v2/spanner-custom-shard/target/spanner-custom-shard-1.0-SNAPSHOT.jar"; | ||
| } |
There was a problem hiding this comment.
The custom shard jar path is hardcoded to an absolute local path specific to a developer's machine. This will cause the integration test to fail in CI/CD environments or on other developers' machines. Use a system property with a relative path fallback instead.
| private String getCustomShardJarPath() { | |
| return "/home/dhwanilpatel_google_com/MyStorage/OracleSupport/DataflowTemplates/v2/spanner-custom-shard/target/spanner-custom-shard-1.0-SNAPSHOT.jar"; | |
| } | |
| private String getCustomShardJarPath() { | |
| return System.getProperty("customShardJarPath", "../spanner-custom-shard/target/spanner-custom-shard-1.0-SNAPSHOT.jar"); | |
| } |
| ConditionCheck sendDataCondition = new ConditionCheck() { | ||
| @Override | ||
| public String getDescription() { | ||
| return "Insert data into Oracle and flush logs"; | ||
| } | ||
|
|
||
| @Override | ||
| protected CheckResult check() { | ||
| try { | ||
| oracleResourceManager.runSQLUpdate("INSERT INTO \"person1\" (\"ID\", \"first_name1\", \"last_name1\") VALUES (1, 'John', 'Doe')"); | ||
| oracleResourceManager.runSQLUpdate("INSERT INTO \"person1\" (\"ID\", \"first_name1\", \"last_name1\") VALUES (2, 'Alice', 'Johnson')"); | ||
|
|
||
| try (java.sql.Connection conn = java.sql.DriverManager.getConnection( | ||
| "jdbc:oracle:thin:@" + System.getProperty("hostIp", "localhost") + ":1521/XEPDB1", "system", "TestPassword123"); | ||
| java.sql.Statement stmt = conn.createStatement()) { | ||
| flushOracleRedoLogs(null); | ||
| } | ||
| return new CheckResult(true, "Data inserted and logs flushed"); | ||
| } catch (Exception e) { | ||
| LOG.error("Failed to insert data or flush logs", e); | ||
| return new CheckResult(false, e.getMessage()); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
The sendDataCondition check does not have an executed or inserted flag. Since check() is called repeatedly until the condition is met, it will attempt to run the INSERT statements multiple times, leading to unique constraint violations (ORA-00001) on the ID primary key. Introduce inserted and flushed flags to prevent duplicate executions.
ConditionCheck sendDataCondition = new ConditionCheck() {
private boolean inserted = false;
private boolean flushed = false;
@Override
public String getDescription() {
return "Insert data into Oracle and flush logs";
}
@Override
protected CheckResult check() {
try {
if (!inserted) {
oracleResourceManager.runSQLUpdate("INSERT INTO \"person1\" (\"ID\", \"first_name1\", \"last_name1\") VALUES (1, 'John', 'Doe')");
oracleResourceManager.runSQLUpdate("INSERT INTO \"person1\" (\"ID\", \"first_name1\", \"last_name1\") VALUES (2, 'Alice', 'Johnson')");
inserted = true;
}
if (!flushed) {
try (java.sql.Connection conn = java.sql.DriverManager.getConnection(
"jdbc:oracle:thin:@" + System.getProperty("hostIp", "localhost") + ":1521/XEPDB1", "system", "TestPassword123");
java.sql.Statement stmt = conn.createStatement()) {
flushOracleRedoLogs(null);
}
flushed = true;
}
return new CheckResult(true, "Data inserted and logs flushed");
} catch (Exception e) {
LOG.error("Failed to insert data or flush logs", e);
return new CheckResult(false, e.getMessage());
}
}
};| CloudOracleResourceManager sysUser = setUpOracleResourceManager(); | ||
|
|
||
| org.apache.beam.it.gcp.cloudsql.CloudOracleResourceManager.Builder sysBuilder = | ||
| CloudOracleResourceManager.builder(testName); | ||
| sysBuilder.setHost(sysUser.getHost()); | ||
| sysBuilder.setPort(sysUser.getPort()); | ||
| sysBuilder.setUsername("sys as sysdba"); | ||
| sysBuilder.setPassword(System.getProperty("cloudProxyPassword")); | ||
| sysBuilder.setDatabaseName(sysUser.getDatabaseName()); | ||
| CloudOracleResourceManager trueSysUser = (CloudOracleResourceManager) sysBuilder.build(); |
There was a problem hiding this comment.
The sysUser and trueSysUser resource managers are instantiated to retrieve connection properties but are never closed or registered for cleanup, which will leak database connections. Additionally, trueSysUser is built but never used. We should use a try-with-resources block to safely close sysUser after extracting the properties, and remove the unused trueSysUser resource manager.
| CloudOracleResourceManager sysUser = setUpOracleResourceManager(); | |
| org.apache.beam.it.gcp.cloudsql.CloudOracleResourceManager.Builder sysBuilder = | |
| CloudOracleResourceManager.builder(testName); | |
| sysBuilder.setHost(sysUser.getHost()); | |
| sysBuilder.setPort(sysUser.getPort()); | |
| sysBuilder.setUsername("sys as sysdba"); | |
| sysBuilder.setPassword(System.getProperty("cloudProxyPassword")); | |
| sysBuilder.setDatabaseName(sysUser.getDatabaseName()); | |
| CloudOracleResourceManager trueSysUser = (CloudOracleResourceManager) sysBuilder.build(); | |
| String host; | |
| int port; | |
| String databaseName; | |
| try (CloudOracleResourceManager sysUser = setUpOracleResourceManager()) { | |
| host = sysUser.getHost(); | |
| port = sysUser.getPort(); | |
| databaseName = sysUser.getDatabaseName(); | |
| } | |
| org.apache.beam.it.gcp.cloudsql.CloudOracleResourceManager.Builder sysBuilder = | |
| CloudOracleResourceManager.builder(testName); | |
| sysBuilder.setHost(host); | |
| sysBuilder.setPort(port); | |
| sysBuilder.setUsername("sys as sysdba"); | |
| sysBuilder.setPassword(System.getProperty("cloudProxyPassword", "TestPassword123")); | |
| sysBuilder.setDatabaseName(databaseName); |
| jdbcResourceManagerShardA, | ||
| jdbcResourceManagerShardA, |
There was a problem hiding this comment.
The jdbcResourceManagerShardA resource manager is passed twice to cleanResources, which is redundant.
| jdbcResourceManagerShardA, | |
| jdbcResourceManagerShardA, | |
| jdbcResourceManagerShardA, |
| public static void cleanUp() throws IOException { | ||
| for (OracleDataStreamToSpannerFileOverridesIT instance : testInstances) { | ||
| instance.tearDownBase(); | ||
| } | ||
| ResourceManagerUtils.cleanResources( | ||
| oracleResourceManager, |
There was a problem hiding this comment.
The oracleSysUser resource manager is instantiated in setUp() but is never cleaned up in cleanUp(), which will leak database connections. Add it to cleanResources.
ResourceManagerUtils.cleanResources(
oracleSysUser,
oracleResourceManager,
spannerResourceManager,
gcsResourceManager,
pubsubResourceManager,
datastreamResourceManager);| public static void cleanUp() throws IOException { | ||
| LOG.info("Cleaning up resources..."); | ||
| for (OracleDatastreamToSpannerDataTypesIT instance : testInstances) { | ||
| instance.tearDownBase(); | ||
| } | ||
| ResourceManagerUtils.cleanResources( |
There was a problem hiding this comment.
The oracleSysUser resource manager is instantiated in setUp() but is never cleaned up in cleanUp(), which will leak database connections. Add it to cleanResources.
ResourceManagerUtils.cleanResources(
oracleSysUser,
oracleResourceManager,
spannerResourceManager,
gcsResourceManager,
pubsubResourceManager,
datastreamResourceManager);| spannerResourceManager, pubsubResourceManager, gcsResourceManager, jdbcResourceManagerShardA, datastreamResourceManager); | ||
| } | ||
|
|
||
| @Test | ||
| public void multiShardMigration() throws Exception { | ||
|
|
There was a problem hiding this comment.
The cloudOracleSysUser resource manager is instantiated in setUp() but is never cleaned up in cleanUp(), which will leak database connections. Add it to cleanResources.
ResourceManagerUtils.cleanResources(
spannerResourceManager,
pubsubResourceManager,
gcsResourceManager,
jdbcResourceManagerShardA,
datastreamResourceManager,
cloudOracleSysUser);
d99bdba to
fb5da77
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4172 +/- ##
=============================================
+ Coverage 35.83% 61.89% +26.06%
- Complexity 711 3444 +2733
=============================================
Files 250 580 +330
Lines 17131 34839 +17708
Branches 1750 3869 +2119
=============================================
+ Hits 6139 21564 +15425
- Misses 10479 12167 +1688
- Partials 513 1108 +595
🚀 New features to boost your workflow:
|
19aa3b3 to
cab3f81
Compare
cab3f81 to
ab8a59e
Compare
1 participant
Adding multiple live template ITs for the oracle source.