[GitHub] [hive] dengzhhu653 commented on a change in pull request #1946: HIVE-24739: Clarify Usage of Thrift TServerEventHandler and Count Number of Messages Processed

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

dengzhhu653 commented on a change in pull request #1946:
URL: https://github.com/apache/hive/pull/1946#discussion_r592854564



##########
File path: service/src/java/org/apache/hive/service/cli/thrift/ThriftBinaryCLIService.java
##########
@@ -113,43 +113,68 @@ protected void initServer() {
       // TCP Server
       server = new TThreadPoolServer(sargs);
       server.setServerEventHandler(new TServerEventHandler() {
+
         @Override
         public ServerContext createContext(TProtocol input, TProtocol output) {
           Metrics metrics = MetricsFactory.getInstance();
           if (metrics != null) {
-            try {
-              metrics.incrementCounter(MetricsConstant.OPEN_CONNECTIONS);
-              metrics.incrementCounter(MetricsConstant.CUMULATIVE_CONNECTION_COUNT);
-            } catch (Exception e) {
-              LOG.warn("Error Reporting JDO operation to Metrics system", e);
-            }
+            metrics.incrementCounter(MetricsConstant.OPEN_CONNECTIONS);
+            metrics.incrementCounter(MetricsConstant.CUMULATIVE_CONNECTION_COUNT);
           }
           return new ThriftCLIServerContext();
         }
 
+        /**
+         * This is called by the Thrift server when the underlying client
+         * connection is cleaned up by the server because the connection has
+         * been closed.
+         */
         @Override
         public void deleteContext(ServerContext serverContext, TProtocol input, TProtocol output) {
           Metrics metrics = MetricsFactory.getInstance();
           if (metrics != null) {
-            try {
-              metrics.decrementCounter(MetricsConstant.OPEN_CONNECTIONS);
-            } catch (Exception e) {
-              LOG.warn("Error Reporting JDO operation to Metrics system", e);
-            }
+            metrics.decrementCounter(MetricsConstant.OPEN_CONNECTIONS);
           }
-          ThriftCLIServerContext context = (ThriftCLIServerContext) serverContext;
-          SessionHandle sessionHandle = context.getSessionHandle();
-          if (sessionHandle != null) {
-            LOG.info("Session disconnected without closing properly. ");
+
+          final ThriftCLIServerContext context = (ThriftCLIServerContext) serverContext;
+          final Optional<SessionHandle> sessionHandle = context.getSessionHandle();
+
+          if (sessionHandle.isPresent()) {
+            // Normally, the client should politely inform the server it is
+            // closing its session with Hive before closing its network
+            // connection. However, if the client connection dies for any reason
+            // (load-balancer round-robin configuration, firewall kills
+            // long-running sessions, bad client, failed client, timed-out
+            // client, etc.) then the server will close the connection without
+            // having properly cleaned up the Hive session (resources,
+            // configuration, logging etc.). That needs to be cleaned up now.
+            LOG.warn(
+                "Client connection bound to {} unexpectedly closed: closing this Hive session to release its resources. "
+                    + "The connection processed {} total messages during its lifetime of {}ms. Inspect the client connection "
+                    + "for time-out, firewall killing the connection, invalid load balancer configuration, etc.",
+                sessionHandle, context.getMessagesProcessedCount(), context.getDuration().toMillis());
             try {
-              boolean close = cliService.getSessionManager().getSession(sessionHandle).getHiveConf()
+              final boolean close = cliService.getSessionManager().getSession(sessionHandle.get()).getHiveConf()
                   .getBoolVar(ConfVars.HIVE_SERVER2_CLOSE_SESSION_ON_DISCONNECT);
-              LOG.info((close ? "" : "Not ") + "Closing the session: " + sessionHandle);
               if (close) {
-                cliService.closeSession(sessionHandle);
+                cliService.closeSession(sessionHandle.get());
+              } else {
+                LOG.warn("Session not actually closed because configuration {} is set to false",
+                    ConfVars.HIVE_SERVER2_CLOSE_SESSION_ON_DISCONNECT.varname);
               }
             } catch (HiveSQLException e) {
-              LOG.warn("Failed to close session: " + e, e);
+              LOG.warn("Failed to close session", e);
+            }
+          } else {
+            // There is no session handle because the client gracefully closed
+            // the session *or* because the client had some issue and never was
+            // able to create one in the first place
+            if (!context.getSessionHandle().isPresent()) {

Review comment:
       nit: ```context.getSessionHandle().isPresent()``` may always return false here when executing the  ```else``` branch...
   
   For connection exiting with ```CloseSession```, the ```sessionHandle``` is cleared before the thrift server calling the ``deleteContext```,  the logging here may be something confusing... 




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