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 2020/10/13 08:58:47 UTC
[GitHub] [spark] gaborgsomogyi opened a new pull request #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
gaborgsomogyi opened a new pull request #30024:
URL: https://github.com/apache/spark/pull/30024
### What changes were proposed in this pull request?
Postgres and MSSQL connection providers are not able to get custom `appEntry` because under some circumstances the driver is wrapped with `DriverWrapper`. Such case is not handled in the mentioned providers. In this PR I've added this edge case handling.
### Why are the changes needed?
`DriverWrapper` is not considered.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing + additional unit tests.
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-712174954
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-711952476
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34605/
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #30024:
URL: https://github.com/apache/spark/pull/30024#discussion_r508201461
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/jdbc/DriverRegistrySuite.scala
##########
@@ -0,0 +1,29 @@
+/*
+ * 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
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.sql.execution.datasources.jdbc.connection.TestDriver
+
+class DriverRegistrySuite extends SparkFunSuite {
+ test("SPARK-32229: get must give back wrapped driver if wrapped") {
Review comment:
If it's just a simple refactoring, we wouldn't need a 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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-707600133
**[Test build #129737 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129737/testReport)** for PR 30024 at commit [`a58b723`](https://github.com/apache/spark/commit/a58b7231ad3c8c92894a2d268543cfd4b34575a0).
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-707740079
Merged build finished. Test PASSed.
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
gaborgsomogyi commented on a change in pull request #30024:
URL: https://github.com/apache/spark/pull/30024#discussion_r504785743
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/connection/MSSQLConnectionProvider.scala
##########
@@ -35,10 +35,12 @@ private[sql] class MSSQLConnectionProvider extends SecureConnectionProvider {
val configName = "jaasConfigurationName"
val appEntryDefault = "SQLJDBCDriver"
+ val wrappedDriver = DriverRegistry.getWrappedDriver(driver)
Review comment:
I think it's doable but have to take a deeper look at SPARK-6913 to be sure.
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-712173336
**[Test build #129998 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129998/testReport)** for PR 30024 at commit [`3d7463f`](https://github.com/apache/spark/commit/3d7463f8476c4ffa67a39c2a26e124cd49a951f1).
* This patch passes all 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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-707624142
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34343/
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
gaborgsomogyi commented on a change in pull request #30024:
URL: https://github.com/apache/spark/pull/30024#discussion_r507575125
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/connection/MSSQLConnectionProvider.scala
##########
@@ -35,10 +35,12 @@ private[sql] class MSSQLConnectionProvider extends SecureConnectionProvider {
val configName = "jaasConfigurationName"
val appEntryDefault = "SQLJDBCDriver"
+ val wrappedDriver = DriverRegistry.getWrappedDriver(driver)
Review comment:
After had a look it play a role only in class loading so we can pass the unwrapped `Driver` right from the beginning. I've changed the approach in the last commit. 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.
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] HeartSaVioR commented on a change in pull request #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on a change in pull request #30024:
URL: https://github.com/apache/spark/pull/30024#discussion_r508204618
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/DriverRegistry.scala
##########
@@ -58,5 +59,15 @@ object DriverRegistry extends Logging {
}
}
}
+
+ def get(className: String): Driver = {
Review comment:
This block actually contains one-line change:
`case d: DriverWrapper if d.wrapped.getClass.getCanonicalName == className => d.wrapped`
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-707740079
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-707633098
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-711990888
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-707740097
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/129737/
Test PASSed.
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-707633098
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-711871509
**[Test build #129998 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129998/testReport)** for PR 30024 at commit [`3d7463f`](https://github.com/apache/spark/commit/3d7463f8476c4ffa67a39c2a26e124cd49a951f1).
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-711951424
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-711905990
Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34604/
----------------------------------------------------------------
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.
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] srowen commented on a change in pull request #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
srowen commented on a change in pull request #30024:
URL: https://github.com/apache/spark/pull/30024#discussion_r504695866
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/connection/MSSQLConnectionProvider.scala
##########
@@ -35,10 +35,12 @@ private[sql] class MSSQLConnectionProvider extends SecureConnectionProvider {
val configName = "jaasConfigurationName"
val appEntryDefault = "SQLJDBCDriver"
+ val wrappedDriver = DriverRegistry.getWrappedDriver(driver)
Review comment:
Seems fine to me. Yeah is there any harm in always trying to unwrap?
----------------------------------------------------------------
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.
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 pull request #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-712619695
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #30024:
URL: https://github.com/apache/spark/pull/30024#discussion_r508171138
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/DriverRegistry.scala
##########
@@ -58,5 +59,15 @@ object DriverRegistry extends Logging {
}
}
}
+
+ def get(className: String): Driver = {
Review comment:
The change seems okay but why do we need to do? It just moves the codes 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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-707738785
**[Test build #129737 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129737/testReport)** for PR 30024 at commit [`a58b723`](https://github.com/apache/spark/commit/a58b7231ad3c8c92894a2d268543cfd4b34575a0).
* This patch passes all 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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-711871509
**[Test build #129998 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129998/testReport)** for PR 30024 at commit [`3d7463f`](https://github.com/apache/spark/commit/3d7463f8476c4ffa67a39c2a26e124cd49a951f1).
----------------------------------------------------------------
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.
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 closed pull request #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
maropu closed pull request #30024:
URL: https://github.com/apache/spark/pull/30024
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
gaborgsomogyi commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-707740362
cc @HyukjinKwon @maropu
----------------------------------------------------------------
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.
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 pull request #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-712619100
LGTM. Thanks for the fix, @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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-711990821
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34605/
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
gaborgsomogyi commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-712632860
Thank you all! Started to work on the documentation PR but this will take some time...
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-711990888
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-707600133
**[Test build #129737 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129737/testReport)** for PR 30024 at commit [`a58b723`](https://github.com/apache/spark/commit/a58b7231ad3c8c92894a2d268543cfd4b34575a0).
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-711951424
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-707633074
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34343/
----------------------------------------------------------------
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.
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] HeartSaVioR commented on a change in pull request #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on a change in pull request #30024:
URL: https://github.com/apache/spark/pull/30024#discussion_r508180571
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/DriverRegistry.scala
##########
@@ -58,5 +59,15 @@ object DriverRegistry extends Logging {
}
}
}
+
+ def get(className: String): Driver = {
Review comment:
I'm actually in favor of this change - DriverRegistry deals with wrapping on register, and this will also let DriverRegistry deal with unwrapping on get. JdbcUtils no longer needs to know about these details - it just needs to know that it should use `DriverRegistry` instead of `DriverManager`.
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/DriverRegistry.scala
##########
@@ -58,5 +59,15 @@ object DriverRegistry extends Logging {
}
}
}
+
+ def get(className: String): Driver = {
Review comment:
I'm actually in favor of this change - `DriverRegistry` deals with wrapping on register, and this will also let `DriverRegistry` deal with unwrapping on get. `JdbcUtils` no longer needs to know about these details - it just needs to know that it should use `DriverRegistry` instead of `DriverManager`.
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-712174954
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #30024:
URL: https://github.com/apache/spark/pull/30024#discussion_r508201158
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/DriverRegistry.scala
##########
@@ -58,5 +59,15 @@ object DriverRegistry extends Logging {
}
}
}
+
+ def get(className: String): Driver = {
Review comment:
I am okay either way but I more mean that I wanted to know how it relates to the PR description/title and JIRA
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
gaborgsomogyi commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-711975100
Scala 2.13 build issue is unrelated:
```
Error: download error: Caught java.io.IOException: Server returned HTTP response code: 504 for URL: https://dl.bintray.com/sbt/sbt-plugin-releases/com.cavorite/sbt-avro/scala_2.12/sbt_1.0/2.1.1/ivys/ivy.xml (Server returned HTTP response code: 504 for URL: https://dl.bintray.com/sbt/sbt-plugin-releases/com.cavorite/sbt-avro/scala_2.12/sbt_1.0/2.1.1/ivys/ivy.xml) while downloading https://repo.scala-sbt.org/scalasbt/sbt-plugin-releases/com.cavorite/sbt-avro/scala_2.12/sbt_1.0/2.1.1/ivys/ivy.xml
```
----------------------------------------------------------------
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.
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] HeartSaVioR commented on a change in pull request #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on a change in pull request #30024:
URL: https://github.com/apache/spark/pull/30024#discussion_r508181273
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/DriverRegistry.scala
##########
@@ -58,5 +59,15 @@ object DriverRegistry extends Logging {
}
}
}
+
+ def get(className: String): Driver = {
Review comment:
That said, why don't we look up `wrapperMap` before iterating through `DriverManager.getDrivers`?
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #30024:
URL: https://github.com/apache/spark/pull/30024#issuecomment-711951392
Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34604/
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #30024:
URL: https://github.com/apache/spark/pull/30024#discussion_r508216007
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/DriverRegistry.scala
##########
@@ -58,5 +59,15 @@ object DriverRegistry extends Logging {
}
}
}
+
+ def get(className: String): Driver = {
Review comment:
oh! got it
----------------------------------------------------------------
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.
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 #30024: [SPARK-32229][SQL]Fix PostgresConnectionProvider and MSSQLConnectionProvider by accessing wrapped driver
Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #30024:
URL: https://github.com/apache/spark/pull/30024#discussion_r504682836
##########
File path: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/connection/MSSQLConnectionProvider.scala
##########
@@ -35,10 +35,12 @@ private[sql] class MSSQLConnectionProvider extends SecureConnectionProvider {
val configName = "jaasConfigurationName"
val appEntryDefault = "SQLJDBCDriver"
+ val wrappedDriver = DriverRegistry.getWrappedDriver(driver)
Review comment:
We cannot move this unwrap code in the caller side `ConnectionProvider.create`? If this issue can happen only in mssql and pg, the current code looks okay. But, I'm not sure about if that is true. IMO it seems safer to always unwrap drivers.
----------------------------------------------------------------
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.
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