[SPARK-47931][SQL] Remove unused and leaked threadlocal/session sessionHive#46153
[SPARK-47931][SQL] Remove unused and leaked threadlocal/session sessionHive#46153yaooqinn wants to merge 1 commit intoapache:masterfrom
Conversation
|
cc @dongjoon-hyun @cloud-fan thanks |
| if (sessionHive != null) { | ||
| Hive.set(sessionHive); | ||
| } | ||
| } |
There was a problem hiding this comment.
I thought HiveSessionImplwithUGI need to override this because of the impersonation. Do you mean HiveSessionImpl's implementation is enough to handle UGI use case?
There was a problem hiding this comment.
UGI is set by setSessionUGI？
| super.acquire(userAccess); | ||
| // if we have a metastore connection with impersonation, then set it first | ||
| if (sessionHive != null) { | ||
| Hive.set(sessionHive); |
There was a problem hiding this comment.
does it have side effects?
There was a problem hiding this comment.
Hive.set(sessionHive) this code looks like having side effects. Is it really safe to remove it?
There was a problem hiding this comment.
In the Hive repo, it implements this logic in its superclass HiveSessionImpl. In the Spark repo, we have this in HiveSessionImplwithUGI only but we mostly use HiveSessionImpl w/o this.
The Hive metadata operations rely upon this, but on the spark side, the metadata operations have been overridden with catalog APIs.
|
cc @sunchao, @LuciferYang , @pan3793 , too |
|
also cc @wangyum FYI |
|
Thank you @LuciferYang and all. Merged to master. |
4 participants
What changes were proposed in this pull request?
This
sessionHiveis never used and properly closedWhy are the changes needed?
A thread local hive instance like
sessionHiveshall be closed properly via 'Hive.closeCurrent'; Otherwise, There will be HMS connection leaks. Here we just remove it as we never use it for HMS connectivities.Does this PR introduce any user-facing change?
no
How was this patch tested?
pass compilation
Was this patch authored or co-authored using generative AI tooling?
no