sadikovi commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r671064138
##########
File path:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/connection/ConnectionProvider.scala
##########
@@ -55,21 +56,48 @@ private[jdbc] object ConnectionProvider extends Logging {
providers.filterNot(p => disabledProviders.contains(p.name)).toSeq
}
- def create(driver: Driver, options: Map[String, String]): Connection = {
+ def create(
+ driver: Driver,
+ options: Map[String, String],
+ connectionProviderName: Option[String]): Connection = {
val filteredProviders = providers.filter(_.canHandle(driver, options))
- require(filteredProviders.size == 1,
- "JDBC connection initiated but not exactly one connection provider found
which can handle " +
- s"it. Found active providers: ${filteredProviders.mkString(", ")}")
+
+ if (filteredProviders.isEmpty) {
+ throw new IllegalArgumentException(
+ "Empty list of JDBC connection providers for the specified driver and
options")
+ }
+
+ val selectedProvider = connectionProviderName match {
+ case Some(providerName) =>
+ // It is assumed that no two providers will have the same name
+ filteredProviders.filter(_.name == providerName).headOption.getOrElse {
+ throw new IllegalArgumentException(
+ s"Could not find a JDBC connection provider with name
'$providerName' " +
+ "that can handle the specified driver and options. " +
+ s"Available providers are ${providers.mkString("[", ", ", "]")}")
+ }
+ case None =>
+ if (filteredProviders.size != 1) {
+ throw new IllegalArgumentException(
+ "JDBC connection initiated but more than one connection provider
was found. Use " +
+ s"'${JDBCOptions.JDBC_CONNECTION_PROVIDER}' option to select a
specific provider. " +
+ s"Found active providers ${filteredProviders.mkString("[", ", ",
"]")}")
+ }
+ filteredProviders.head
+ }
+
SecurityConfigurationLock.synchronized {
// Inside getConnection it's safe to get parent again because
SecurityConfigurationLock
// makes sure it's untouched
val parent = Configuration.getConfiguration
try {
- filteredProviders.head.getConnection(driver, options)
+ selectedProvider.getConnection(driver, options)
} finally {
logDebug("Restoring original security configuration")
Configuration.setConfiguration(parent)
}
}
}
}
+
+private[jdbc] object ConnectionProvider extends ConnectionProviderBase
Review comment:
I thought about it. The reason I decided to go with a separate class is
ability to add new methods that rely on providers list and testing of such
methods fully. My concern was adding a custom logic in `create()` method which
would not be tested by `doCreate()`. Also this should mitigate a potential
confusion how providers list is used in the class.
##########
File path:
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/jdbc/connection/ConnectionProviderSuite.scala
##########
@@ -45,6 +54,84 @@ class ConnectionProviderSuite extends
ConnectionProviderSuiteBase with SharedSpa
}
}
+ test("Throw an error selecting from an empty list of providers on create") {
Review comment:
Unfortunately, `DISABLED_JDBC_CONN_PROVIDER_LIST` is a static conf and
`providers` is not lazy, i.e. I am not sure how to reevaluate providers in
ConnectionProvider once the class has been loaded without reflection. See the
comment below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
