You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2022/01/24 14:19:58 UTC
[GitHub] [hive] dengzhhu653 opened a new pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
dengzhhu653 opened a new pull request #2967:
URL: https://github.com/apache/hive/pull/2967
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2967:
URL: https://github.com/apache/hive/pull/2967#discussion_r802471573
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandlerContext.java
##########
@@ -0,0 +1,136 @@
+/*
+ * 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<>();
Review comment:
Make sense, Thank you for the suggestion, @kgyrtkirk!
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvary commented on pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2967:
URL: https://github.com/apache/hive/pull/2967#issuecomment-1020861747
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.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on pull request #2967:
URL: https://github.com/apache/hive/pull/2967#issuecomment-1024840583
Hi @pvary, could you please take another look if have some secs? Thank you!
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvary commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2967:
URL: https://github.com/apache/hive/pull/2967#discussion_r791581995
##########
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:
This is not a `cleanupRawStore` for a good while...
How widely used is this? Could we rename this?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
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!
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2967:
URL: https://github.com/apache/hive/pull/2967#discussion_r791730792
##########
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #2967:
URL: https://github.com/apache/hive/pull/2967#discussion_r802395259
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -608,34 +549,21 @@ private void authorizeTableForPartitionMetadata(
isServerFilterEnabled, filterHook, catName, dbName, tblName);
}
- private static String addPrefix(String s) {
Review comment:
it seems like log messages were prefixed with the `threadLocalId` - which is being removed here; or its being retained differently?
* its already displayed in the name of the thread..
* or it doesnt really add much value as the thread's name should already be in the logs...
history takes me back HIVE-1818 and HIVE-1081 ; so the original intention is not that clear
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandlerContext.java
##########
@@ -0,0 +1,136 @@
+/*
+ * 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<>();
Review comment:
I think we may create a new instance of the context in `initialValue()` and remove quite a few conditionals from the getters
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] kgyrtkirk commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on a change in pull request #2967:
URL: https://github.com/apache/hive/pull/2967#discussion_r802407329
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -608,34 +549,21 @@ private void authorizeTableForPartitionMetadata(
isServerFilterEnabled, filterHook, catName, dbName, tblName);
}
- private static String addPrefix(String s) {
Review comment:
I see the other thread - lets remove threadid
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2967:
URL: https://github.com/apache/hive/pull/2967#discussion_r802481062
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandlerContext.java
##########
@@ -0,0 +1,136 @@
+/*
+ * 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<>();
Review comment:
Done
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2967:
URL: https://github.com/apache/hive/pull/2967#discussion_r791676766
##########
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.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2967:
URL: https://github.com/apache/hive/pull/2967#discussion_r791630926
##########
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.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvary commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2967:
URL: https://github.com/apache/hive/pull/2967#discussion_r791690315
##########
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:
Makes sense. Then we will be able to remove the ThreadId threadlocal
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvary commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2967:
URL: https://github.com/apache/hive/pull/2967#discussion_r791618884
##########
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:
Adding `threadId` to the log might not be needed. Our stack is configured so the logger automatically logs the threadId. This should be so in every environment where the threading is a serious issue, so I think this is not necessary.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvary commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2967:
URL: https://github.com/apache/hive/pull/2967#discussion_r791642104
##########
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:
Is `addPrefix` used anywhere in the code after this?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvary commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2967:
URL: https://github.com/apache/hive/pull/2967#discussion_r791585817
##########
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:
Feels a bit like overengineering to me.
Why not just do this the `cleanupHook` stuff outside, and clear the values here?
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
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 `cleanupHandlerContext` with public modifier? Thank you for the suggestions!
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2967:
URL: https://github.com/apache/hive/pull/2967#discussion_r791598568
##########
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
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvary commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2967:
URL: https://github.com/apache/hive/pull/2967#discussion_r791652057
##########
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:
I think the threadId is not used in the RawStoreProxy
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on pull request #2967:
URL: https://github.com/apache/hive/pull/2967#issuecomment-1020924354
> 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.
Add a [CleanupHook](https://github.com/apache/hive/blob/c8c6152b79810276ac070ec057b0643b9ce3b03f/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandlerContext.java#L170-L172) here to clear the locals by calling HMSHandlerContext#clear(CleanupHook cleanupHook), not sure it enough for the iceberg case.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvary commented on pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2967:
URL: https://github.com/apache/hive/pull/2967#issuecomment-1020861747
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.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvary commented on pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
pvary commented on pull request #2967:
URL: https://github.com/apache/hive/pull/2967#issuecomment-1031263338
@dengzhhu653: Sorry, I was out of town last week.
LGTM
@kgyrtkirk, or @nrg4878 would you like to take a look?
Thanks,
Peter
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on pull request #2967:
URL: https://github.com/apache/hive/pull/2967#issuecomment-1020924354
> 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.
Add a [CleanupHook](https://github.com/apache/hive/blob/c8c6152b79810276ac070ec057b0643b9ce3b03f/standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandlerContext.java#L170-L172) here to clear the locals by calling HMSHandlerContext#clear(CleanupHook cleanupHook), not sure it enough for the iceberg case.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
dengzhhu653 commented on a change in pull request #2967:
URL: https://github.com/apache/hive/pull/2967#discussion_r791646932
##########
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.
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] pvary commented on a change in pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
pvary commented on a change in pull request #2967:
URL: https://github.com/apache/hive/pull/2967#discussion_r791581995
##########
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:
This is not a `cleanupRawStore` for a good while...
How widely used is this? Could we rename this?
##########
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:
Feels a bit like overengineering to me.
Why not just do this the `cleanupHook` stuff outside, and clear the values here?
##########
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:
Adding `threadId` to the log might not be needed. Our stack is configured so the logger automatically logs the threadId. This should be so in every environment where the threading is a serious issue, so I think this is not necessary.
##########
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:
Is `addPrefix` used anywhere in the code after this?
##########
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:
I think the threadId is not used in the RawStoreProxy
##########
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:
Makes sense. Then we will be able to remove the ThreadId threadlocal
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org
[GitHub] [hive] dengzhhu653 merged pull request #2967: HIVE-25892: Group HMSHandler's thread locals into a single context
Posted by GitBox <gi...@apache.org>.
dengzhhu653 merged pull request #2967:
URL: https://github.com/apache/hive/pull/2967
--
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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org