#752 Remove 'driver' from JDBC options, and try current thread context class loader for dynamically loaded JDBC drivers.#753
Conversation
…t class loader for dynamically loaded JDBC drivers.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThis pull request defers JDBC driver loading from initialization to connection time and adds fallback logic. The driver property is removed from connection properties, and driver loading is wrapped in a try/catch that retries using the thread context class loader if the default loader fails. ChangesJDBC Driver Loading Deferral
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Unit Test Coverage
Files
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pramen/core/src/main/scala/za/co/absa/pramen/core/utils/JdbcNativeUtils.scala (1)
213-214: ⚡ Quick winRedundant class loading.
Class.forName(jdbcConfig.driver, true, loader)already loads and initializes the class, but its return value is discarded. The subsequentloader.loadClass(jdbcConfig.driver)call loads the same class again.♻️ Proposed fix to use the return value directly
- Class.forName(jdbcConfig.driver, true, loader) - val driverClass = loader.loadClass(jdbcConfig.driver) + val driverClass = Class.forName(jdbcConfig.driver, true, loader)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pramen/core/src/main/scala/za/co/absa/pramen/core/utils/JdbcNativeUtils.scala` around lines 213 - 214, The code redundantly loads the JDBC driver twice; remove the extra loader.loadClass call and use the Class.forName return instead: replace the two lines so that Class.forName(jdbcConfig.driver, true, loader) is assigned to driverClass (or otherwise use its returned Class object) and then proceed with existing logic that uses driverClass; update references to driverClass accordingly and keep jdbcConfig.driver and loader as the identifying symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@pramen/core/src/main/scala/za/co/absa/pramen/core/utils/JdbcNativeUtils.scala`:
- Line 217: In JdbcNativeUtils (look for the driver.connect(url, properties)
call), add a null-check after Driver.connect to handle the case where the driver
declines the URL: if the returned connection is null, throw a clear
IllegalStateException (or SQLException) with a message mentioning the URL and
driver class instead of proceeding to use the null `connection` (which leads to
an NPE at connection.setAutoCommit); this ensures code paths in methods that use
the `connection` variable (e.g., wherever `autoCommit` is set) either get a
valid Connection or a descriptive error.
---
Nitpick comments:
In
`@pramen/core/src/main/scala/za/co/absa/pramen/core/utils/JdbcNativeUtils.scala`:
- Around line 213-214: The code redundantly loads the JDBC driver twice; remove
the extra loader.loadClass call and use the Class.forName return instead:
replace the two lines so that Class.forName(jdbcConfig.driver, true, loader) is
assigned to driverClass (or otherwise use its returned Class object) and then
proceed with existing logic that uses driverClass; update references to
driverClass accordingly and keep jdbcConfig.driver and loader as the identifying
symbols.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 338c801e-154a-48a9-b4ba-f42d02a7abdc
📒 Files selected for processing (2)
pramen/core/src/main/scala/za/co/absa/pramen/core/reader/JdbcUrlSelectorImpl.scalapramen/core/src/main/scala/za/co/absa/pramen/core/utils/JdbcNativeUtils.scala
💤 Files with no reviewable changes (1)
- pramen/core/src/main/scala/za/co/absa/pramen/core/reader/JdbcUrlSelectorImpl.scala
…ses appropriately (thenks @coderabbitai)
Summary by CodeRabbit