[GitHub] [hive] kishendas commented on a change in pull request #2238: HIVE-25085: MetaStore Clients no longer shared across sessions.

[GitBox <gi...@apache.org>]

kishendas commented on a change in pull request #2238:
URL: https://github.com/apache/hive/pull/2238#discussion_r630589315



##########
File path: service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java
##########
@@ -408,18 +400,10 @@ private synchronized void acquireAfterOpLock(boolean userAccess) {
     // set the thread name with the logging prefix.
     sessionState.updateThreadName();
 
-    // If the thread local Hive is different from sessionHive, it means, the previous query execution in
-    // master thread has re-created Hive object due to changes in MS related configurations in sessionConf.
-    // So, it is necessary to reset sessionHive object based on new sessionConf. Here, we cannot,
-    // directly set sessionHive with thread local Hive because if the previous command was REPL LOAD, then
-    // the config changes lives only within command execution not in session level.
-    // So, the safer option is to invoke Hive.get() which decides if to reuse Thread local Hive or re-create it.
-    if (Hive.getThreadLocal() != sessionHive) {
-      try {
-        setSessionHive();
-      } catch (HiveSQLException e) {
-        throw new RuntimeException(e);
-      }
+    try {

Review comment:
       Please avoid formatting only changes.

##########
File path: ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
##########
@@ -1869,7 +1874,10 @@ public void close() throws IOException {
       unCacheDataNucleusClassLoaders();
     } finally {
       // removes the threadlocal variables, closes underlying HMS connection
-      Hive.closeCurrent();
+      if (hiveDb != null) {
+        hiveDb.close(true);
+        hiveDb = null;

Review comment:
       Why do you have to set it to null ?

##########
File path: service/src/java/org/apache/hive/service/cli/session/HiveSessionImpl.java
##########
@@ -240,21 +240,13 @@ protected int processCmd(String cmd) {
    * @throws HiveSQLException
    */
   private void setSessionHive() throws HiveSQLException {
-    Hive newSessionHive;
     try {
-      newSessionHive = Hive.get(getHiveConf());
-
-      // HMS connections from sessionHive shouldn't be closed by any query execution thread when it
-      // recreates the Hive object. It is allowed to be closed only when session is closed/released.
-      newSessionHive.setAllowClose(false);
+      sessionHive = sessionState.getHiveDb();
     } catch (HiveException e) {
-      throw new HiveSQLException("Failed to get metastore connection", e);

Review comment:
       Can you not just do - LOG.error("Failed to retrieve Hive object", e); ?

##########
File path: ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
##########
@@ -1869,7 +1874,10 @@ public void close() throws IOException {
       unCacheDataNucleusClassLoaders();
     } finally {
       // removes the threadlocal variables, closes underlying HMS connection
-      Hive.closeCurrent();
+      if (hiveDb != null) {
+        hiveDb.close(true);

Review comment:
       Will it not fail this because of hiveDb.setAllowClose(false); elsewhere ?

##########
File path: ql/src/java/org/apache/hadoop/hive/ql/session/SessionState.java
##########
@@ -2210,6 +2218,18 @@ public void endScope(String queryId) {
   public Map<Object, Object> getQueryCache(String queryId) {
     return cache.get(queryId);
   }
+
+  public Hive getHiveDb() throws HiveException {
+    if (hiveDb == null) {
+      hiveDb = Hive.createHiveForSession(sessionConf);
+      // Need to setAllowClose to false. For legacy reasons, the Hive object is stored
+      // in thread local storage. If allowClose is true, the session can get closed when
+      // the thread goes away which is not desirable when the Hive object is used across
+      // different queries in the session.
+      hiveDb.setAllowClose(false);

Review comment:
       What happens if we try to close ? Will it throw exception or it will be a no-op ?




-- 
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.

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