Repository navigation
Merge features from the support branch to the main branch #824
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
06b0499
9400b89
4a36afc
9a457e7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -26,6 +26,7 @@ import za.co.absa.pramen.core.utils.JdbcNativeUtils.JDBC_WORDS_TO_REDACT | |||||||||||
| import za.co.absa.pramen.core.utils.{ConfigUtils, JdbcNativeUtils} | ||||||||||||
|
|
||||||||||||
| import java.time.LocalDate | ||||||||||||
| import scala.util.{Failure, Success} | ||||||||||||
|
|
||||||||||||
| abstract class TableReaderJdbcBase(jdbcReaderConfig: TableReaderJdbcConfig, | ||||||||||||
| jdbcUrlSelector: JdbcUrlSelector, | ||||||||||||
|
|
@@ -43,18 +44,36 @@ abstract class TableReaderJdbcBase(jdbcReaderConfig: TableReaderJdbcConfig, | |||||||||||
| val gen = SqlGeneratorLoader.getSqlGenerator(jdbcReaderConfig.jdbcConfig.driver, getSqlConfig) | ||||||||||||
|
|
||||||||||||
| if (gen.requiresConnection) { | ||||||||||||
| val (connection, _) = jdbcUrlSelector.getNewConnection(jdbcRetries) | ||||||||||||
| val (connection, _) = jdbcUrlSelector.getConnection | ||||||||||||
| gen.setConnection(connection) | ||||||||||||
| } | ||||||||||||
| gen | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private[core] lazy val serverTimeZone = { | ||||||||||||
| jdbcReaderConfig.serverTimeZone match { | ||||||||||||
| case Some(tz) => | ||||||||||||
| log.info(s"The server timezone is specified explicitly: $tz") | ||||||||||||
| tz | ||||||||||||
| case None => | ||||||||||||
| val (connection, _) = jdbcUrlSelector.getConnection | ||||||||||||
| sqlGen.getServerTimezone(connection) match { | ||||||||||||
|
Comment on lines
+59
to
+60
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
fd -i 'JdbcUrlSelector.scala' --exec cat -n {}
rg -nP --type=scala -C3 '\bgetConnection\b|\bgetNewConnection\b' pramen/core/src/mainRepository: AbsaOSS/pramen Length of output: 24759 🏁 Script executed: sed -n '1,125p' pramen/core/src/main/scala/za/co/absa/pramen/core/reader/TableReaderJdbcBase.scala
sed -n '1,210p' pramen/core/src/main/scala/za/co/absa/pramen/core/reader/JdbcUrlSelectorImpl.scala
rg -n -F --glob '*.scala' -e 'jdbcRetries' -e 'serverTimeZone' -e 'jdbcUrlSelector.close' pramen/core/src/mainRepository: AbsaOSS/pramen Length of output: 19815 Include connection acquisition in the timezone fallback. When no timezone is configured, 🐛 Suggested fix--- "a/pramen/core/src/main/scala/za/co/absa/pramen/core/reader/TableReaderJdbcBase.scala"
+++ "b/pramen/core/src/main/scala/za/co/absa/pramen/core/reader/TableReaderJdbcBase.scala"
@@ -56,8 +56,9 @@
log.info(s"The server timezone is specified explicitly: $tz")
tz
case None =>
- val (connection, _) = jdbcUrlSelector.getConnection
- sqlGen.getServerTimezone(connection) match {
+ scala.util.Try(jdbcUrlSelector.getConnection).flatMap { case (connection, _) =>
+ sqlGen.getServerTimezone(connection)
+ } match {
case Success(tz) =>
log.info(s"The server timezone is detected as: $tz")
tz
📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||
| case Success(tz) => | ||||||||||||
| log.info(s"The server timezone is detected as: $tz") | ||||||||||||
| tz | ||||||||||||
| case Failure(ex) => | ||||||||||||
| log.warn(s"Failed to detect the server timezone: '${ex.getMessage}'. Using the default timezone: ${jdbcReaderConfig.defaultTimeZone}") | ||||||||||||
| jdbcReaderConfig.defaultTimeZone | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| private[core] def getSqlConfig: SqlConfig = { | ||||||||||||
| SqlConfig(jdbcReaderConfig.infoDateColumn, | ||||||||||||
| jdbcReaderConfig.infoDateType, | ||||||||||||
| jdbcReaderConfig.infoDateFormat, | ||||||||||||
| jdbcReaderConfig.offsetInfoOpt, | ||||||||||||
| jdbcReaderConfig.serverTimeZone, | ||||||||||||
| jdbcReaderConfig.serverTimeZone.getOrElse(jdbcReaderConfig.defaultTimeZone), | ||||||||||||
| jdbcReaderConfig.identifierQuotingPolicy, | ||||||||||||
| jdbcReaderConfig.sqlGeneratorClass, | ||||||||||||
| ConfigUtils.getExtraConfig(conf, "sql")) | ||||||||||||
|
|
||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: AbsaOSS/pramen
Length of output: 12141
🏁 Script executed:
Repository: AbsaOSS/pramen
Length of output: 41901
🏁 Script executed:
Repository: AbsaOSS/pramen
Length of output: 6624
🏁 Script executed:
Repository: AbsaOSS/pramen
Length of output: 3846
Add defaults for the new timezone members, but document the signature change as breaking.
The default
FailureforSqlGenerator.getServerTimezonedoes not preserve direct custom implementations that still use the oldgetDataQueryIncrementalsignature. It only removes the new abstract-method requirement.SqlGeneratorBaseprovides the new incremental method itself, so subclasses that inherit it are not affected by that signature change. However, the new abstractgetServerTimezoneQuerybreaks existing subclasses that do not implement it. Give that member an unsupported default. Do not imply that this fixes the changed incremental-query signature for direct implementations or subclasses that override it.Document the
getDataQueryIncrementalsignature change in the release notes.Suggested fix
🤖 Prompt for AI Agents