HIVE-25892: Group HMSHandler's thread locals into a single context #2967
Conversation
|
Could we provide a way to clear the context? In Iceberg we create a HMS instance for our tests, and because of the threadlocals here and in the TxnHandler we leak information between the tests. |
dengzhhu653
force-pushed
the
HIVE-25892
branch
from
80494d8
to
c8c6152
Compare
Add a CleanupHook here to clear the locals by calling HMSHandlerContext#clear(CleanupHook cleanupHook), not sure it enough for the iceberg case. |
| public static RawStore getRawStore() { | ||
| return threadLocalMS.get(); | ||
| return HMSHandlerContext.getRawStore().orElse(null); | ||
| } | ||
|
|
||
| static void cleanupRawStore() { |
There was a problem hiding this comment.
This is not a cleanupRawStore for a good while...
How widely used is this? Could we rename this?
There was a problem hiding this comment.
It's been used only in two places(HMSHandler&HiveMetaStore) when client disconnects，can we rename it to cleanupHandlerContext with public modifier? Thank you for the suggestions!
| public static void clear(CleanupHook cleanupHook) { | ||
| HMSHandlerContext ctx = context.get(); | ||
| context.remove(); | ||
| if (ctx != null && cleanupHook != null) { |
There was a problem hiding this comment.
Feels a bit like overengineering to me.
Why not just do this the cleanupHook stuff outside, and clear the values here?
There was a problem hiding this comment.
OK, let me take a look. Thanks
...ne-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
Outdated
Show resolved
Hide resolved
| LOG.info(addPrefix("Opening raw store with implementation class:" + rawStoreClassName)); | ||
| return RawStoreProxy.getProxy(newConf, conf, rawStoreClassName, threadLocalId.get()); | ||
| LOG.info("Opening raw store with implementation class: {}", rawStoreClassName); | ||
| return RawStoreProxy.getProxy(newConf, conf, rawStoreClassName, HMSHandlerContext.getThreadId()); |
There was a problem hiding this comment.
I think the threadId is not used in the RawStoreProxy
There was a problem hiding this comment.
yes, i was thinking if we can remove the getThreadId from IHMSHandler, which is declared as InterfaceAudience.Private, the getThreadId is only used for logging now, how about raising another pr for this? I also want to improve the metrics and audit of the HMSHandler #2441.
There was a problem hiding this comment.
Makes sense. Then we will be able to remove the ThreadId threadlocal
There was a problem hiding this comment.
Got it, I opened https://issues.apache.org/jira/browse/HIVE-25896 for the removal of getThreadId
kgyrtkirk
added
tests pending
tests passed
and removed
tests failed
tests pending
labels
|
Hi @pvary, could you please take another look if have some secs? Thank you! |
|
@dengzhhu653: Sorry, I was out of town last week. @kgyrtkirk, or @nrg4878 would you like to take a look? Thanks, |
| */ | ||
| public final class HMSHandlerContext { | ||
|
|
||
| private static final ThreadLocal<HMSHandlerContext> context = new ThreadLocal<>(); |
There was a problem hiding this comment.
I think we may create a new instance of the context in initialValue() and remove quite a few conditionals from the getters
There was a problem hiding this comment.
Make sense, Thank you for the suggestion, @kgyrtkirk!
...ne-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
Show resolved
Hide resolved
…pache#2967) (Zhihua Deng, reviewed by Peter Vary and Zoltan Haindrich)
…pache#2967) (Zhihua Deng, reviewed by Peter Vary and Zoltan Haindrich)
What changes were proposed in this pull request?
Why are the changes needed?
Does this PR introduce any user-facing change?
How was this patch tested?