[SPARK-22394] [SQL] Remove redundant synchronization for metastore access #19605
Conversation
|
Test build #83202 has finished for PR 19605 at commit
|
| * `clientLoader` in the `retryLocked` method. | ||
| */ | ||
| private def withClient[T](body: => T): T = synchronized { | ||
| private def withClient[T](body: => T): T = { |
There was a problem hiding this comment.
If you check the callers of withClient, you can find many callers conduct multiple client-related operations in a single body. Removing this lock might cause some concurrency issues.
There was a problem hiding this comment.
Then can we remove the synchronization of clientLoader in HiveClientImpl? If we synchronize withClient, then there's no need to sync each operation in body.
There was a problem hiding this comment.
but please make sure we only use the hive client here.
There was a problem hiding this comment.
Please check whether all the Hive client calls are through the Hive External Catalog. For example, addJar is an exception.
There was a problem hiding this comment.
I went through all methods in HiveClient having synchronization (except addJar):
getStateis used only in test.setOut,setInfoandsetErrorare only used inSparkSQLEnv.init().- all other methods are called through
HiveExternalCatalog.
So it seems addJar is the only exception.
To make addJar also go throught HiveExternalCatalog, we can pass externalCatalog instead of client at line46 in HiveSessionStateBuilder. But I don't know why we need to call newSession() at line45, where a new HiveClientImpl instance is created, with the same class loader and Hive client.
There was a problem hiding this comment.
Last year, we had a discussion about whether addJar should be moved to HiveExternalCatalog. See #14883
|
Also cc @srinathshankar @JoshRosen @hvanhovell @zsxwing |
|
@wzhfy Let us close this PR first. We can restart the discussion when we exposing ExternalCatalog APIs. Thanks! |
4 participants
What changes were proposed in this pull request?
Before Spark 2.x, synchronization for metastore access was protected at line229 in ClientWrapper (now it's at line203 in HiveClientWrapper ).
After Spark 2.x,
HiveExternalCatalogwas introduced by SPARK-13080, where an extra level of synchronization was added at line95. That is, now we have two levels of synchronization: one isHiveExternalCatalogand the other isIsolatedClientLoaderinHiveClientImpl.But since both
HiveExternalCatalogandIsolatedClientLoaderare shared among all spark sessions, the extra level of synchronization inHiveExternalCatalogis redundant, thus can be removed.How was this patch tested?
Manual test and existing tests.