Skip to content

Commit 271aa33

Browse files
sarutakdongjoon-hyun
authored andcommitted
[MINOR][SQL] Refactor the comments in HiveClientImpl.withHiveState
### What changes were proposed in this pull request? This PR refactors three parts of the comments in `HiveClientImpl.withHiveState` One is about the following comment. ``` // The classloader in clientLoader could be changed after addJar, always use the latest // classloader. ``` The comment was added in SPARK-10810 (apache#8909) because `IsolatedClientLoader.classLoader` was declared as `var`. But the field is now `val` and cannot be changed after instanciation. So, the comment can confuse developers. One is about the following code and comment. ``` // classloader. We explicitly set the context class loader since "conf.setClassLoader" does // not do that, and the Hive client libraries may need to load classes defined by the client's // class loader. Thread.currentThread().setContextClassLoader(clientLoader.classLoader) ``` It's not trivial why this part is necessary and it's difficult when we can remove this code in the future. So, I revised the comment by adding the reference of the related JIRA. And the last one is about the following code and comment. ``` // Replace conf in the thread local Hive with current conf Hive.get(conf) ``` It's also not trivial why this part is necessary. I revised the comment by adding the reference of the related discussion. ### Why are the changes needed? To make code more readable. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? It's just a comment refactoring so I add no new test. Closes apache#32162 from sarutak/refactor-HiveClientImpl. Authored-by: Kousuke Saruta <sarutak@oss.nttdata.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
1 parent 8a3815f commit 271aa33

File tree

1 file changed

+5
-3
lines changed

1 file changed

+5
-3
lines changed

sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -292,15 +292,17 @@ private[hive] class HiveClientImpl(
292292
def withHiveState[A](f: => A): A = retryLocked {
293293
val original = Thread.currentThread().getContextClassLoader
294294
val originalConfLoader = state.getConf.getClassLoader
295-
// The classloader in clientLoader could be changed after addJar, always use the latest
296-
// classloader. We explicitly set the context class loader since "conf.setClassLoader" does
295+
// We explicitly set the context class loader since "conf.setClassLoader" does
297296
// not do that, and the Hive client libraries may need to load classes defined by the client's
298-
// class loader.
297+
// class loader. See SPARK-19804 for more details.
299298
Thread.currentThread().setContextClassLoader(clientLoader.classLoader)
300299
state.getConf.setClassLoader(clientLoader.classLoader)
301300
// Set the thread local metastore client to the client associated with this HiveClientImpl.
302301
Hive.set(client)
303302
// Replace conf in the thread local Hive with current conf
303+
// with the side-effect of Hive.get(conf) to avoid using out-of-date HiveConf.
304+
// See discussion in https://github.com/apache/spark/pull/16826/files#r104606859
305+
// for more details.
304306
Hive.get(conf)
305307
// setCurrentSessionState will use the classLoader associated
306308
// with the HiveConf in `state` to override the context class loader of the current

0 commit comments

Comments
 (0)