Add oracle bulk ITs - #4161
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 adds support for Oracle database integration testing within the SourceDbToSpanner template. It includes the necessary infrastructure in the base test class to manage Oracle resources and introduces a new integration test to validate the migration pipeline using a simple schema. Highlights
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 introduces Oracle database support to the SourceDbToSpanner integration tests, including helper methods in the base class and a new simple integration test OracleSourceDbToSpannerSimpleIT. Feedback focuses on preventing resource leaks in database connections by using try-with-resources, avoiding shaded imports, removing unused fields, and converting static test fields to instance variables to prevent shared state issues.
| try { | ||
| Connection connection = | ||
| DriverManager.getConnection( | ||
| jdbcResourceManager.getUri(), | ||
| jdbcResourceManager.getUsername(), | ||
| jdbcResourceManager.getPassword()); | ||
|
|
||
| // Preprocess SQL to handle multi-line statements and newlines | ||
| sql = sql.replaceAll("\r\n", " ").replaceAll("\n", " "); | ||
|
|
||
| // Split into individual statements based on -- SPLIT -- | ||
| String[] statements = sql.split("-- SPLIT --"); | ||
|
|
||
| // Execute each statement | ||
| Statement statement = connection.createStatement(); | ||
| for (String stmt : statements) { | ||
| if (!stmt.trim().isEmpty()) { | ||
| LOG.info("Executing Oracle statement: {}", stmt); | ||
| statement.executeUpdate(stmt); | ||
| } | ||
| } | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
The Connection and Statement resources are opened but never closed, which causes a resource leak. Use a try-with-resources block to ensure they are properly closed after execution. Additionally, using DriverManager.getConnection directly is preferred for obtaining single, short-lived database connections to avoid leaking connection pools.
try (Connection connection =
DriverManager.getConnection(
jdbcResourceManager.getUri(),
jdbcResourceManager.getUsername(),
jdbcResourceManager.getPassword());
Statement statement = connection.createStatement()) {
// Preprocess SQL to handle multi-line statements and newlines
String preprocessedSql = sql.replaceAll("\r\n", " ").replaceAll("\n", " ");
// Split into individual statements based on -- SPLIT --
String[] statements = preprocessedSql.split("-- SPLIT --");
// Execute each statement
for (String stmt : statements) {
if (!stmt.trim().isEmpty()) {
LOG.info("Executing Oracle statement: {}", stmt);
statement.executeUpdate(stmt);
}
}
} catch (Exception e) {References
- Do not instantiate a connection pool (such as HikariDataSource) to obtain single, short-lived database connections, as it can leak the connection pool if the datasource is not closed. Use DriverManager.getConnection directly instead.
| import org.junit.runners.JUnit4; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.testcontainers.shaded.com.google.common.collect.ImmutableMap; |
There was a problem hiding this comment.
Avoid importing shaded classes from org.testcontainers.shaded. Use the standard Guava ImmutableMap instead.
| import org.testcontainers.shaded.com.google.common.collect.ImmutableMap; | |
| import com.google.common.collect.ImmutableMap; |
| private static HashSet<OracleSourceDbToSpannerSimpleIT> testInstances = new HashSet<>(); | ||
| private static PipelineLauncher.LaunchInfo jobInfo; |
There was a problem hiding this comment.
The testInstances field is unused and should be removed. Additionally, jobInfo does not need to be static since it is only used within instance test methods.
| private static HashSet<OracleSourceDbToSpannerSimpleIT> testInstances = new HashSet<>(); | |
| private static PipelineLauncher.LaunchInfo jobInfo; | |
| private PipelineLauncher.LaunchInfo jobInfo; |
| public static OracleResourceManager oracleResourceManager; | ||
| public static SpannerResourceManager spannerResourceManager; |
There was a problem hiding this comment.
These resource managers are initialized in @Before and cleaned up in @After for each test execution. They should be instance variables (non-static) to prevent potential issues with shared state or flakiness if tests are run in parallel.
| public static OracleResourceManager oracleResourceManager; | |
| public static SpannerResourceManager spannerResourceManager; | |
| public OracleResourceManager oracleResourceManager; | |
| public SpannerResourceManager spannerResourceManager; |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (16.05%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #4161 +/- ##
=============================================
+ Coverage 35.83% 61.70% +25.86%
- Complexity 711 3444 +2733
=============================================
Files 250 581 +331
Lines 17131 34959 +17828
Branches 1750 3878 +2128
=============================================
+ Hits 6139 21571 +15432
- Misses 10479 12279 +1800
- Partials 513 1109 +596
🚀 New features to boost your workflow:
|
1ee7a1e to
0240639
Compare
19f55bf to
cb2dbd0
Compare
648c72d to
2d38b49
Compare
2d38b49 to
54b46f2
Compare
1 participant
Adding Oracle ITs for bulk template.