dengzhhu653 commented on a change in pull request #2967:
URL: https://github.com/apache/hive/pull/2967#discussion_r791589935
##########
File path:
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -169,65 +169,29 @@
private Warehouse wh; // hdfs warehouse
private static Striped<Lock> tablelocks;
- private static final ThreadLocal<RawStore> threadLocalMS = new
ThreadLocal<RawStore>();
- private static final ThreadLocal<TxnStore> threadLocalTxn = new
ThreadLocal<TxnStore>();
-
- private static final ThreadLocal<Map<String,
com.codahale.metrics.Timer.Context>> timerContexts =
- new ThreadLocal<Map<String, com.codahale.metrics.Timer.Context>>() {
- @Override
- protected Map<String, com.codahale.metrics.Timer.Context>
initialValue() {
- return new HashMap<>();
- }
- };
-
public static RawStore getRawStore() {
- return threadLocalMS.get();
+ return HMSHandlerContext.getRawStore().orElse(null);
}
static void cleanupRawStore() {
Review comment:
It's been used only in two places(HMSHandler&HiveMetaStore) when client
disconnects，can we rename it to `cleanupContext` with public modifier? Thank
you for the suggestions!
##########
File path:
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -169,65 +169,29 @@
private Warehouse wh; // hdfs warehouse
private static Striped<Lock> tablelocks;
- private static final ThreadLocal<RawStore> threadLocalMS = new
ThreadLocal<RawStore>();
- private static final ThreadLocal<TxnStore> threadLocalTxn = new
ThreadLocal<TxnStore>();
-
- private static final ThreadLocal<Map<String,
com.codahale.metrics.Timer.Context>> timerContexts =
- new ThreadLocal<Map<String, com.codahale.metrics.Timer.Context>>() {
- @Override
- protected Map<String, com.codahale.metrics.Timer.Context>
initialValue() {
- return new HashMap<>();
- }
- };
-
public static RawStore getRawStore() {
- return threadLocalMS.get();
+ return HMSHandlerContext.getRawStore().orElse(null);
}
static void cleanupRawStore() {
Review 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!
##########
File path:
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandlerContext.java
##########
@@ -0,0 +1,173 @@
+/*
+ * 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.hadoop.hive.metastore;
+
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hive.metastore.txn.TxnStore;
+import org.apache.hadoop.hive.metastore.txn.TxnUtils;
+
+/**
+ * When one hms client connects in, we create a handler context for it.
+ * We store session information here.
+ */
+public final class HMSHandlerContext {
+
+ private static final ThreadLocal<HMSHandlerContext> context = new
ThreadLocal<>();
+
+ private static final AtomicInteger nextSerialNum = new AtomicInteger();
+
+ private RawStore rawStore;
+
+ private TxnStore txnStore;
+
+ // Thread local HMSHandler used during shutdown to notify meta listeners
+ private HMSHandler hmsHandler;
+
+ // Thread local configuration is needed as many threads could make changes
+ // to the conf using the connection hook
+ private Configuration configuration;
+
+ // Thread local Map to keep track of modified meta conf keys
+ private Map<String, String> modifiedConfig = new HashMap<>();
+
+ private Integer threadId = nextSerialNum.incrementAndGet();
+ // This will only be set if the metastore is being accessed from a metastore
Thrift server,
+ // not if it is from the CLI. Also, only if the TTransport being used to
connect is an
+ // instance of TSocket. This is also not set when kerberos is used.
+ private String ipAddress;
+
+ private Map<String, com.codahale.metrics.Timer.Context> timerContexts = new
HashMap<>();
+
+ private HMSHandlerContext() {
+
+ }
+
+ public static Optional<RawStore> getRawStore() {
+ HMSHandlerContext ctx = context.get();
+ return ctx != null ? ctx.getLocalRawStore() : Optional.empty();
+ }
+
+ public static Optional<HMSHandler> getHMSHandler() {
+ HMSHandlerContext ctx = context.get();
+ return ctx != null ? ctx.getLocalHmsHandler() : Optional.empty();
+ }
+
+ public static Optional<String> getIpAddress() {
+ HMSHandlerContext ctx = context.get();
+ return ctx != null ? ctx.getRemoteIpAddress() : Optional.empty();
+ }
+
+ public static Optional<Configuration> getConfiguration() {
+ HMSHandlerContext ctx = context.get();
+ return ctx != null ? ctx.getLocalConfiguration() : Optional.empty();
+ }
+
+ public static TxnStore getTxnStore(Configuration conf) {
+ return getContext().getLocalTxnStore().orElseGet(() -> {
+ TxnStore txnStore = TxnUtils.getTxnStore(conf);
+ setTxnStore(txnStore);
+ return txnStore;
+ });
+ }
+
+ public static Map<String, String> getModifiedConfig() {
+ return getContext().modifiedConfig;
+ }
+
+ public static Integer getThreadId() {
+ return getContext().threadId;
+ }
+
+ public static Map<String, com.codahale.metrics.Timer.Context>
getTimerContexts() {
+ return getContext().timerContexts;
+ }
+
+ private static HMSHandlerContext getContext() {
+ HMSHandlerContext ctx = context.get();
+ if (ctx == null) {
+ context.set(ctx = new HMSHandlerContext());
+ }
+ return ctx;
+ }
+
+ public static void setRawStore(RawStore rawStore) {
+ getContext().rawStore = rawStore;
+ }
+
+ public static void setTxnStore(TxnStore txnStore) {
+ getContext().txnStore = txnStore;
+ }
+
+ public static void setHMSHandler(HMSHandler hmsHandler) {
+ getContext().hmsHandler = hmsHandler;
+ }
+
+ public static void setConfiguration(Configuration conf) {
+ getContext().configuration = conf;
+ }
+
+ public static void setIpAddress(String ipAddress) {
+ getContext().ipAddress = ipAddress;
+ }
+
+ public static void clear(CleanupHook cleanupHook) {
+ HMSHandlerContext ctx = context.get();
+ context.remove();
+ if (ctx != null && cleanupHook != null) {
Review comment:
OK, let me take a look. Thanks
##########
File path:
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -705,32 +635,22 @@ public static RawStore getMSForConf(Configuration conf)
throws MetaException {
ms.shutdown();
throw e;
}
- threadLocalMS.set(ms);
- ms = threadLocalMS.get();
- LOG.info("Created RawStore: " + ms + " from thread id: " +
Thread.currentThread().getId());
+ HMSHandlerContext.setRawStore(ms);
+ LOG.info("Created RawStore: " + ms + " from thread id: " +
HMSHandlerContext.getThreadId());
}
return ms;
}
@Override
public TxnStore getTxnHandler() {
- return getMsThreadTxnHandler(conf);
- }
-
- public static TxnStore getMsThreadTxnHandler(Configuration conf) {
- TxnStore txn = threadLocalTxn.get();
- if (txn == null) {
- txn = TxnUtils.getTxnStore(conf);
- threadLocalTxn.set(txn);
- }
- return txn;
+ return HMSHandlerContext.getTxnStore(conf);
}
static RawStore newRawStoreForConf(Configuration conf) throws MetaException {
Configuration newConf = new Configuration(conf);
String rawStoreClassName = MetastoreConf.getVar(newConf,
ConfVars.RAW_STORE_IMPL);
- 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: {}",
HMSHandlerContext.getThreadId(), rawStoreClassName);
Review comment:
The `addPrefix` method will insert the threadId before the log, unsure
why do like this. It makes sense for me to remove the threadId from the log.
##########
File path:
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -705,32 +635,22 @@ public static RawStore getMSForConf(Configuration conf)
throws MetaException {
ms.shutdown();
throw e;
}
- threadLocalMS.set(ms);
- ms = threadLocalMS.get();
- LOG.info("Created RawStore: " + ms + " from thread id: " +
Thread.currentThread().getId());
+ HMSHandlerContext.setRawStore(ms);
+ LOG.info("Created RawStore: " + ms + " from thread id: " +
HMSHandlerContext.getThreadId());
}
return ms;
}
@Override
public TxnStore getTxnHandler() {
- return getMsThreadTxnHandler(conf);
- }
-
- public static TxnStore getMsThreadTxnHandler(Configuration conf) {
- TxnStore txn = threadLocalTxn.get();
- if (txn == null) {
- txn = TxnUtils.getTxnStore(conf);
- threadLocalTxn.set(txn);
- }
- return txn;
+ return HMSHandlerContext.getTxnStore(conf);
}
static RawStore newRawStoreForConf(Configuration conf) throws MetaException {
Configuration newConf = new Configuration(conf);
String rawStoreClassName = MetastoreConf.getVar(newConf,
ConfVars.RAW_STORE_IMPL);
- 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: {}",
HMSHandlerContext.getThreadId(), rawStoreClassName);
Review comment:
No, I have removed this method in this fix.
##########
File path:
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -705,32 +635,22 @@ public static RawStore getMSForConf(Configuration conf)
throws MetaException {
ms.shutdown();
throw e;
}
- threadLocalMS.set(ms);
- ms = threadLocalMS.get();
- LOG.info("Created RawStore: " + ms + " from thread id: " +
Thread.currentThread().getId());
+ HMSHandlerContext.setRawStore(ms);
+ LOG.info("Created RawStore: {}", ms);
}
return ms;
}
@Override
public TxnStore getTxnHandler() {
- return getMsThreadTxnHandler(conf);
- }
-
- public static TxnStore getMsThreadTxnHandler(Configuration conf) {
- TxnStore txn = threadLocalTxn.get();
- if (txn == null) {
- txn = TxnUtils.getTxnStore(conf);
- threadLocalTxn.set(txn);
- }
- return txn;
+ return HMSHandlerContext.getTxnStore(conf);
}
static RawStore newRawStoreForConf(Configuration conf) throws MetaException {
Configuration newConf = new Configuration(conf);
String rawStoreClassName = MetastoreConf.getVar(newConf,
ConfVars.RAW_STORE_IMPL);
- 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());
Review comment:
yes, i was thinking if we can remove the
[getThreadId](https://github.com/apache/hive/blob/master/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/IHMSHandler.java#L43-L47)
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
https://github.com/apache/hive/pull/2441.
##########
File path:
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -705,32 +635,22 @@ public static RawStore getMSForConf(Configuration conf)
throws MetaException {
ms.shutdown();
throw e;
}
- threadLocalMS.set(ms);
- ms = threadLocalMS.get();
- LOG.info("Created RawStore: " + ms + " from thread id: " +
Thread.currentThread().getId());
+ HMSHandlerContext.setRawStore(ms);
+ LOG.info("Created RawStore: {}", ms);
}
return ms;
}
@Override
public TxnStore getTxnHandler() {
- return getMsThreadTxnHandler(conf);
- }
-
- public static TxnStore getMsThreadTxnHandler(Configuration conf) {
- TxnStore txn = threadLocalTxn.get();
- if (txn == null) {
- txn = TxnUtils.getTxnStore(conf);
- threadLocalTxn.set(txn);
- }
- return txn;
+ return HMSHandlerContext.getTxnStore(conf);
}
static RawStore newRawStoreForConf(Configuration conf) throws MetaException {
Configuration newConf = new Configuration(conf);
String rawStoreClassName = MetastoreConf.getVar(newConf,
ConfVars.RAW_STORE_IMPL);
- 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());
Review comment:
Got it, I opened https://issues.apache.org/jira/browse/HIVE-25896 for
the removal of `getThreadId`
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
