gaborgsomogyi commented on a change in pull request #29024:
URL: https://github.com/apache/spark/pull/29024#discussion_r454212309



##########
File path: 
core/src/main/scala/org/apache/spark/security/SecurityConfigurationLock.scala
##########
@@ -0,0 +1,28 @@
+/*
+ * 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.security
+
+import org.apache.spark.annotation.DeveloperApi
+
+/**
+ * ::DeveloperApi::
+ * There are cases when global JVM security configuration must be modified.
+ * In order to avoid race the modification must be synchronized with this.
+ */
+@DeveloperApi
+object SecurityConfigurationLock

Review comment:
       `Considering this I would not add synchronization into the framework` = 
Adding synchronization into a central place (like `ConnectionProvider.create`) 
and allowing 3rd-party developers not to care about this is not something where 
I see the gain (I see cases where such change would do unnecessary 
synchronization). That said physically it's not an issue but could be 
misleading.
   An example:
   ```
     def create(driver: Driver, options: JDBCOptions): Connection = {
       val filteredProviders = providers.filter(_.canHandle(driver, options))
       logDebug(s"Filtered providers: $filteredProviders")
       require(filteredProviders.size == 1,
         "JDBC connection initiated but not exactly one connection provider 
found which can handle it")
       var conn: Connection = null
       // This would synchronize but for nothing in some cases
       SecurityConfigurationLock.synchronized {
         conn = filteredProviders.head.getConnection(driver, options)
       }
       conn
     }
   ```
   An imaginary provider implemented by 3rd-party:
   ```
   class OracleConnectionProviderTGT {
     override def canHandle(driver: Driver, options: JDBCOptions): Boolean = {
       // Example content of tgtCache: "/tmp/krb5cc_5088"
       options.tgtCache != null ...
     }
   
     override def getConnection(driver: Driver, options: JDBCOptions): 
Connection = {
       ...
       // No need to modify global JVM configuration
       
prop.setProperty(OracleConnection.CONNECTION_PROPERTY_THIN_NET_AUTHENTICATION_KRB5_CC_NAME,
                        options.tgtCache)
       ...
       driver.connect(url, prop)
     }
   }
   ```
   Overall if we would like to add such change then I would mention that 
`getConnection` is synchronized under any circustances which may or may not 
needed. This is not suggested from my perspective but no strong opinion.
   




----------------------------------------------------------------
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:
us...@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

Reply via email to