[GitHub] [hive] yigress commented on a diff in pull request #3621: HIVE-26564: Separate query live operation log and historical operation log

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

yigress commented on code in PR #3621:
URL: https://github.com/apache/hive/pull/3621#discussion_r980701496


##########
service/src/test/org/apache/hive/service/cli/operation/TestOperationLogManager.java:
##########
@@ -61,6 +60,7 @@ public void setUp() throws Exception {
     HiveConf.setIntVar(hiveConf, HiveConf.ConfVars.HIVE_SERVER2_WEBUI_MAX_HISTORIC_QUERIES, 1);
     HiveConf.setIntVar(hiveConf, HiveConf.ConfVars.HIVE_SERVER2_WEBUI_PORT, 8080);
     HiveConf.setBoolVar(hiveConf, HiveConf.ConfVars.HIVE_IN_TEST, true);
+    HiveConf.setBoolVar(hiveConf, HiveConf.ConfVars.HIVE_TESTING_REMOVE_LOGS, false);

Review Comment:
   This is because https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/session/OperationLog.java#L80
       if (hiveConf.getBoolVar(HiveConf.ConfVars.HIVE_IN_TEST)) {
         isRemoveLogs = hiveConf.getBoolVar(HiveConf.ConfVars.HIVE_TESTING_REMOVE_LOGS);
   
   Instead of changing the code there, I thought it may be better set it to false for this test case. In production no need to change this setting.



##########
service/src/java/org/apache/hive/service/cli/operation/Operation.java:
##########
@@ -314,12 +318,13 @@ protected synchronized void cleanupOperationLog(final long operationLogCleanupDe
       } else {
         if (operationLogCleanupDelayMs > 0) {
           ScheduledExecutorService scheduledExecutorService = Executors.newScheduledThreadPool(1);
-          scheduledExecutorService.schedule(new OperationLogCleaner(operationLog), operationLogCleanupDelayMs,
+          scheduledExecutorService.schedule(new OperationLogCleaner(this, operationLog), operationLogCleanupDelayMs,
             TimeUnit.MILLISECONDS);
           scheduledExecutorService.shutdown();
         } else {
           log.info("Closing operation log {} without delay", operationLog);
           operationLog.close();
+          OperationLogManager.closeOperation(this);

Review Comment:
   The reason I did not set inside operationLog.close is that OperationLog is a wrapper of the log file itself and has no  knowledge of the operation. 



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