You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/07/15 14:48:50 UTC
[GitHub] [spark] sadikovi opened a new pull request #33370: [SPARK-36163] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
sadikovi opened a new pull request #33370:
URL: https://github.com/apache/spark/pull/33370
### What changes were proposed in this pull request?
This PR fixes two issues highlighted in https://issues.apache.org/jira/browse/SPARK-36163:
- JDBC connection provider propagates incorrect connection properties.
- Ambiguity when more than one JDBC connection provider is available.
I updated `BasicConnectionProvider` to use `jdbcOptions.asConnectionProperties` to remove JDBC data source specific options.
I also added `connectionProvider` data source option that specifies the name of the provider, e.g. `db2`, `presto`, to allow enforcing this specific provider in case of ambiguity.
### Why are the changes needed?
Users can leverage `spark.sql.sources.disabledJdbcConnProviderList` but it is cumbersome and requires them to disable all other providers which could be problematic when using ambiguous providers in two or more different JDBC queries.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
Yes
PROBLEM DESCRIPTION:
This introduces new JDBC data source option `connectionProvider` that allows users to select a specific JDBC connection provider based on the short name. I updated the SQL guide doc and README.
Before this change, the only way to resolve ambiguity was SQL conf to blacklist all of the other JDBC connection providers. After this change users will be able to specify the exact connection provider they need per data source.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
I updated the existing `ConnectionProviderSuite` and added a new `BasicConnectionProviderSuite`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #33370: [SPARK-36163] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r670888286
##########
File path: docs/sql-data-sources-jdbc.md
##########
@@ -275,11 +275,22 @@ logging into the data sources.
</td>
<td>read/write</td>
</tr>
+
+ <tr>
+ <td><code>connectionProvider</code></td>
Review comment:
I think this option conflicts with `spark.sql.sources.disabledJdbcConnProviderList` (see also https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/jdbc/README.md#how-to-disable-jdbc-connection-providers). This option is an allow list and that specific option is a disallow list.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881538937
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/141147/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881384172
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/45660/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881577658
**[Test build #141167 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141167/testReport)** for PR 33370 at commit [`92194f4`](https://github.com/apache/spark/commit/92194f462640a7c3a9d844b5b7f7cc7a7e817348).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881611659
**[Test build #141170 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141170/testReport)** for PR 33370 at commit [`e94cf0c`](https://github.com/apache/spark/commit/e94cf0cc507fe31f26651587a257a36db16763fc).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881644732
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45679/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
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. 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881538937
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/141147/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-882367353
Merged to master.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881702496
**[Test build #141167 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141167/testReport)** for PR 33370 at commit [`92194f4`](https://github.com/apache/spark/commit/92194f462640a7c3a9d844b5b7f7cc7a7e817348).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881641488
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45682/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881537688
**[Test build #141147 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141147/testReport)** for PR 33370 at commit [`b6f556f`](https://github.com/apache/spark/commit/b6f556f0c7a84243f6b2a7b779cb25d634c54394).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gaborgsomogyi commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
gaborgsomogyi commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r671033161
##########
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") {
+ val providerBase = new ConnectionProviderBase() {
+ override val providers = Seq.empty
+ }
+
+ val err1 = intercept[IllegalArgumentException] {
+ providerBase.create(mock[Driver], Map.empty, None)
+ }
+ assert(err1.getMessage.contains("Empty list of JDBC connection providers"))
+
+ val err2 = intercept[IllegalArgumentException] {
+ providerBase.create(mock[Driver], Map.empty, Some("test"))
+ }
+ assert(err2.getMessage.contains("Empty list of JDBC connection providers"))
+ }
+
+ test("Throw an error when more than one provider is available on create") {
+ val provider1 = new JdbcConnectionProvider() {
Review comment:
I have the feeling that `TestJdbcConnectionProvider(val name, val getConnectionImpl...)` class can be created in a common area. Such way several copy-paste can be eliminated, right?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33370: [SPARK-36163] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-880758566
Can one of the admins verify this patch?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881705187
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/141167/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi edited a comment on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi edited a comment on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881382793
I would appreciate another round of reviews, let me know if changes are okay or you have any concerns or objections - I will be more than happy to address. cc @maropu @gaborgsomogyi
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gaborgsomogyi commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
gaborgsomogyi commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881243580
Basically looks good, left some notes.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881834787
Yeah I think it should better be lazy val
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881573427
"Throw an error if the selected provider is disabled" test fails in CI but passes locally. I will debug.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi edited a comment on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi edited a comment on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881585553
I think there `providers` should be lazy or even a method in ConnectionProvider to allow DISABLED_JDBC_CONN_PROVIDER_LIST to be set on SparkSession creation. cc @HyukjinKwon. It results in different outcome in the test due to different test order. For now, I am going to remove "Throw an error if the selected provider is disabled".
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881384153
Kubernetes integration test unable to build dist.
exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45660/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881611659
**[Test build #141170 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141170/testReport)** for PR 33370 at commit [`e94cf0c`](https://github.com/apache/spark/commit/e94cf0cc507fe31f26651587a257a36db16763fc).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881098929
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/45622/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881644966
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/45679/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881098929
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/45622/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881577658
**[Test build #141167 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141167/testReport)** for PR 33370 at commit [`92194f4`](https://github.com/apache/spark/commit/92194f462640a7c3a9d844b5b7f7cc7a7e817348).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r671137488
##########
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") {
+ val providerBase = new ConnectionProviderBase() {
+ override val providers = Seq.empty
+ }
+
+ val err1 = intercept[IllegalArgumentException] {
+ providerBase.create(mock[Driver], Map.empty, None)
+ }
+ assert(err1.getMessage.contains("Empty list of JDBC connection providers"))
+
+ val err2 = intercept[IllegalArgumentException] {
+ providerBase.create(mock[Driver], Map.empty, Some("test"))
+ }
+ assert(err2.getMessage.contains("Empty list of JDBC connection providers"))
+ }
+
+ test("Throw an error when more than one provider is available on create") {
+ val provider1 = new JdbcConnectionProvider() {
Review comment:
IMHO, I would have to provide closures for `canHandle` and `getConnection` to the class anyway, so I am not quite sure what the test wrapper class would accomplish.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881705187
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/141167/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-882369208
Thank you very much @HyukjinKwon!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r671065547
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/jdbc/connection/BasicConnectionProviderSuite.scala
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.execution.datasources.jdbc.connection
+
+import java.sql.{Connection, Driver}
+import java.util.Properties
+
+import org.mockito.ArgumentMatchers.any
+import org.mockito.Mockito._
+import org.mockito.invocation.InvocationOnMock
+import org.scalatestplus.mockito.MockitoSugar
+
+import org.apache.spark.sql.execution.datasources.jdbc.JDBCOptions
+
+class BasicConnectionProviderSuite extends ConnectionProviderSuiteBase with MockitoSugar {
+ test("Check properties of BasicConnectionProvider") {
+ val opts = options("jdbc:postgresql://localhost/postgres")
+ val provider = new BasicConnectionProvider()
+ assert(provider.name == "basic")
+ assert(provider.getAdditionalProperties(opts).isEmpty())
+ }
+
+ test("Check that JDBC options don't contain data source configs") {
+ val provider = new BasicConnectionProvider()
+ val driver = mock[Driver]
+ when(driver.connect(any(), any())).thenAnswer((invocation: InvocationOnMock) => {
+ val props = invocation.getArguments().apply(1).asInstanceOf[Properties]
+ val conn = mock[Connection]
+ when(conn.getClientInfo()).thenReturn(props)
+ conn
+ })
+
+ val opts = Map(
+ JDBCOptions.JDBC_URL -> "jdbc:postgresql://localhost/postgres",
+ JDBCOptions.JDBC_TABLE_NAME -> "table",
+ JDBCOptions.JDBC_CONNECTION_PROVIDER -> "basic")
+ val conn = provider.getConnection(driver, opts)
+ assert(!conn.getClientInfo().containsKey(JDBCOptions.JDBC_URL))
Review comment:
It was not reported as an issue so I did not check. I think JDBCOptions require `table` that is why it is in the map, I will add another assertion.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881509638
Can one of the admins verify this patch?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r671058808
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/README.md
##########
@@ -46,6 +46,12 @@ so they can be turned off and can be replaced with custom implementation. All CP
which must be unique. One can set the following configuration entry in `SparkConf` to turn off CPs:
Review comment:
I will rebase, thanks.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gaborgsomogyi commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
gaborgsomogyi commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r671028756
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/README.md
##########
@@ -46,6 +46,12 @@ so they can be turned off and can be replaced with custom implementation. All CP
which must be unique. One can set the following configuration entry in `SparkConf` to turn off CPs:
`spark.sql.sources.disabledJdbcConnProviderList=name1,name2`.
+## How to enforce a specific JDBC connection provider?
+
+When more than one JDBC connection provider can handle a specific driver and options, it is possible to
+disambiguate and enforce a particular CP for the JDBC data source. One can set the DataFrameReader
Review comment:
+1 on this question.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881352257
**[Test build #141147 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141147/testReport)** for PR 33370 at commit [`b6f556f`](https://github.com/apache/spark/commit/b6f556f0c7a84243f6b2a7b779cb25d634c54394).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r671396068
##########
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.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881674962
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/45682/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881098911
Kubernetes integration test unable to build dist.
exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45622/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
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 don't think it is a good idea to have a separate method. It would create confusion around how the providers list is generated.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #33370: [SPARK-36163] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r670888827
##########
File path: docs/sql-data-sources-jdbc.md
##########
@@ -275,11 +275,22 @@ logging into the data sources.
</td>
<td>read/write</td>
</tr>
+
+ <tr>
+ <td><code>connectionProvider</code></td>
Review comment:
Oh, okay. I just noticed that you mentioned `spark.sql.sources.disabledJdbcConnProviderList` in the Pr description.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881352257
**[Test build #141147 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141147/testReport)** for PR 33370 at commit [`b6f556f`](https://github.com/apache/spark/commit/b6f556f0c7a84243f6b2a7b779cb25d634c54394).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881627566
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45678/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881384172
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/45660/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881608568
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45678/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] gaborgsomogyi commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
gaborgsomogyi commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r671029910
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/jdbc/connection/BasicConnectionProviderSuite.scala
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.spark.sql.execution.datasources.jdbc.connection
+
+import java.sql.{Connection, Driver}
+import java.util.Properties
+
+import org.mockito.ArgumentMatchers.any
+import org.mockito.Mockito._
+import org.mockito.invocation.InvocationOnMock
+import org.scalatestplus.mockito.MockitoSugar
+
+import org.apache.spark.sql.execution.datasources.jdbc.JDBCOptions
+
+class BasicConnectionProviderSuite extends ConnectionProviderSuiteBase with MockitoSugar {
+ test("Check properties of BasicConnectionProvider") {
+ val opts = options("jdbc:postgresql://localhost/postgres")
+ val provider = new BasicConnectionProvider()
+ assert(provider.name == "basic")
+ assert(provider.getAdditionalProperties(opts).isEmpty())
+ }
+
+ test("Check that JDBC options don't contain data source configs") {
+ val provider = new BasicConnectionProvider()
+ val driver = mock[Driver]
+ when(driver.connect(any(), any())).thenAnswer((invocation: InvocationOnMock) => {
+ val props = invocation.getArguments().apply(1).asInstanceOf[Properties]
+ val conn = mock[Connection]
+ when(conn.getClientInfo()).thenReturn(props)
+ conn
+ })
+
+ val opts = Map(
+ JDBCOptions.JDBC_URL -> "jdbc:postgresql://localhost/postgres",
+ JDBCOptions.JDBC_TABLE_NAME -> "table",
+ JDBCOptions.JDBC_CONNECTION_PROVIDER -> "basic")
+ val conn = provider.getConnection(driver, opts)
+ assert(!conn.getClientInfo().containsKey(JDBCOptions.JDBC_URL))
Review comment:
Why JDBC_TABLE_NAME not checked any way?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r671129801
##########
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:
Added
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
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 don't think it is a good idea to have a separate method. It would create confusion around how the providers list is generated and passed around.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33370: [SPARK-36163] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881098275
**[Test build #141109 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141109/testReport)** for PR 33370 at commit [`ae76767`](https://github.com/apache/spark/commit/ae76767474968f494b2c0632cd39df223f770270).
* This patch **fails to build**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `protected abstract class ConnectionProviderBase extends Logging `
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #33370: [SPARK-36163] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881090909
ok to test
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r670892697
##########
File path: docs/sql-data-sources-jdbc.md
##########
@@ -275,11 +275,22 @@ logging into the data sources.
</td>
<td>read/write</td>
</tr>
+
+ <tr>
+ <td><code>connectionProvider</code></td>
+ <td>(none)</td>
+ <td>
+ The short name of the JDBC connection provider to use to connect to this URL, e.g. <code>db2</code>, <code>mssql</code>.
+ Must be one of the providers loaded with the JDBC data source. Used to disambiguate when more than one provider can handle
+ the specified driver and options.
+ </td>
+ <td>read/write</td>
+ </tr>
Review comment:
nit: the indents look incorrect.
##########
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:
You made this class for testing? Instead of creating an unnecessary class hierarchy, how about just an inner method for testing like this?
```
// For testing
private[jdbc] def doCreate(
driver: Driver,
providers: Seq[JdbcConnectionProvider],
options: Map[String, String]): Connection = {
...
}
def create(driver: Driver, options: Map[String, String]): Connection = {
doCreate(driver, providers, options)
}
```
##########
File path: docs/sql-data-sources-jdbc.md
##########
@@ -275,11 +275,22 @@ logging into the data sources.
</td>
<td>read/write</td>
</tr>
+
+ <tr>
+ <td><code>connectionProvider</code></td>
+ <td>(none)</td>
+ <td>
+ The short name of the JDBC connection provider to use to connect to this URL, e.g. <code>db2</code>, <code>mssql</code>.
+ Must be one of the providers loaded with the JDBC data source. Used to disambiguate when more than one provider can handle
+ the specified driver and options.
Review comment:
How about describing a relationship to `DISABLED_JDBC_CONN_PROVIDER_LIST` here?
##########
File path: docs/sql-data-sources-jdbc.md
##########
@@ -275,11 +275,22 @@ logging into the data sources.
</td>
<td>read/write</td>
</tr>
+
+ <tr>
+ <td><code>connectionProvider</code></td>
+ <td>(none)</td>
+ <td>
+ The short name of the JDBC connection provider to use to connect to this URL, e.g. <code>db2</code>, <code>mssql</code>.
Review comment:
What does `short` means here? `The name of a JDBC connection provider to use ...`?
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/README.md
##########
@@ -46,6 +46,12 @@ so they can be turned off and can be replaced with custom implementation. All CP
which must be unique. One can set the following configuration entry in `SparkConf` to turn off CPs:
`spark.sql.sources.disabledJdbcConnProviderList=name1,name2`.
+## How to enforce a specific JDBC connection provider?
+
+When more than one JDBC connection provider can handle a specific driver and options, it is possible to
+disambiguate and enforce a particular CP for the JDBC data source. One can set the DataFrameReader
Review comment:
Is this related to the reader only? how about the writer?
##########
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:
Could you add some tests for the case where a user setting both `DISABLED_JDBC_CONN_PROVIDER_LIST` and `connectionProvider`?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881755056
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/141170/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi edited a comment on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi edited a comment on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881585553
I think there `providers` should be lazy or even a method in ConnectionProvider to allow DISABLED_JDBC_CONN_PROVIDER_LIST to be set on SparkSession creation. cc @HyukjinKwon. It results in different outcome in the test due to different class loader ordering. For now, I am going to remove "Throw an error if the selected provider is disabled".
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r670921029
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/README.md
##########
@@ -46,6 +46,12 @@ so they can be turned off and can be replaced with custom implementation. All CP
which must be unique. One can set the following configuration entry in `SparkConf` to turn off CPs:
Review comment:
https://github.com/apache/spark/pull/33381
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881674962
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/45682/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33370: [SPARK-36163] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-880758566
Can one of the admins verify this patch?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon closed pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
HyukjinKwon closed pull request #33370:
URL: https://github.com/apache/spark/pull/33370
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881659506
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45682/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33370: [SPARK-36163] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881098292
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/141109/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r671059249
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/jdbc/connection/ConnectionProviderSuite.scala
##########
@@ -17,12 +17,21 @@
package org.apache.spark.sql.execution.datasources.jdbc.connection
+import java.sql.{Connection, Driver}
import javax.security.auth.login.Configuration
+import org.mockito.Mockito._
Review comment:
Yes, I will redesign the tests to work around this issue.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA removed a comment on pull request #33370: [SPARK-36163] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881095785
**[Test build #141109 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141109/testReport)** for PR 33370 at commit [`ae76767`](https://github.com/apache/spark/commit/ae76767474968f494b2c0632cd39df223f770270).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881585553
I think there `providers` should be lazy or even a method in ConnectionProvider to allow DISABLED_JDBC_CONN_PROVIDER_LIST to be set on SparkSession creation to be evaluated. cc @HyukjinKwon. It results in different outcome in the test due to different class loader ordering. For now, I am going to remove "Throw an error if the selected provider is disabled".
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] maropu commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r670905233
##########
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:
You made this class for testing? Instead of creating an unnecessary class hierarchy, how about just creating an inner method for testing like this?
```
// For testing
private[jdbc] def doCreate(
driver: Driver,
providers: Seq[JdbcConnectionProvider],
options: Map[String, String]): Connection = {
...
}
def create(driver: Driver, options: Map[String, String]): Connection = {
doCreate(driver, providers, options)
}
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881382793
I would appreciate another round of reviews, let me know if changes are okay or you have any concerns or objections - I will be more than happy to address.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-882369208
Thank you very much @HyukjinKwon!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33370: [SPARK-36163] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881095785
**[Test build #141109 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141109/testReport)** for PR 33370 at commit [`ae76767`](https://github.com/apache/spark/commit/ae76767474968f494b2c0632cd39df223f770270).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881644966
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/45679/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881746346
**[Test build #141170 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/141170/testReport)** for PR 33370 at commit [`e94cf0c`](https://github.com/apache/spark/commit/e94cf0cc507fe31f26651587a257a36db16763fc).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `class DisallowedConnectionProviderSuite extends SharedSparkSession `
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881755056
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/141170/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r671064424
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/README.md
##########
@@ -46,6 +46,12 @@ so they can be turned off and can be replaced with custom implementation. All CP
which must be unique. One can set the following configuration entry in `SparkConf` to turn off CPs:
`spark.sql.sources.disabledJdbcConnProviderList=name1,name2`.
+## How to enforce a specific JDBC connection provider?
+
+When more than one JDBC connection provider can handle a specific driver and options, it is possible to
+disambiguate and enforce a particular CP for the JDBC data source. One can set the DataFrameReader
Review comment:
It is both. I meant a DataFrame option here.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881345311
Thanks for the reviews. I addressed most of the comments and left my replies to the rest of them.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
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.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins removed a comment on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881642843
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/45678/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #33370: [SPARK-36163] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r670885022
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/jdbc/connection/ConnectionProviderSuite.scala
##########
@@ -17,12 +17,21 @@
package org.apache.spark.sql.execution.datasources.jdbc.connection
+import java.sql.{Connection, Driver}
import javax.security.auth.login.Configuration
+import org.mockito.Mockito._
Review comment:
shall we remove this import:
```
[error] /home/runner/work/spark/spark/sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/jdbc/connection/ConnectionProviderSuite.scala:23:28: Unused import
[error] import org.mockito.Mockito._
[error] ^
[error] one error found
[error] (sql / Test / compileIncremental) Compilation failed
[error] Total time: 599 s (09:59), completed Jul 15, 2021 2:48:41 PM
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on pull request #33370: [SPARK-36163] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-880757890
FYI @maropu
FYI @gaborgsomogyi
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] SparkQA commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881624233
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/45679/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on a change in pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r671059249
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/jdbc/connection/ConnectionProviderSuite.scala
##########
@@ -17,12 +17,21 @@
package org.apache.spark.sql.execution.datasources.jdbc.connection
+import java.sql.{Connection, Driver}
import javax.security.auth.login.Configuration
+import org.mockito.Mockito._
Review comment:
Yes, I will update.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881642843
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/45678/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33370: [SPARK-36163] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881098292
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/141109/
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon closed pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
HyukjinKwon closed pull request #33370:
URL: https://github.com/apache/spark/pull/33370
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-881509638
Can one of the admins verify this patch?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] sadikovi commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
sadikovi commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-882339736
It seems the tests passed! Are there any other comments or suggestions on the changes?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on pull request #33370: [SPARK-36163][SQL] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on pull request #33370:
URL: https://github.com/apache/spark/pull/33370#issuecomment-882367353
Merged to master.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #33370: [SPARK-36163] Propagate correct JDBC properties in JDBC connector provider and add "connectionProvider" option
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #33370:
URL: https://github.com/apache/spark/pull/33370#discussion_r670890080
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/jdbc/README.md
##########
@@ -46,6 +46,12 @@ so they can be turned off and can be replaced with custom implementation. All CP
which must be unique. One can set the following configuration entry in `SparkConf` to turn off CPs:
Review comment:
Actually, this option has to be placed in `StaticSQLConf`, or should be effective in runtime. I will make a separate PR to move it for now.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org