[GitHub] [hive] pvary commented on a change in pull request #1222: HIVE-23814: Clean up Driver (Miklos Gergely)

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

pvary commented on a change in pull request #1222:
URL: https://github.com/apache/hive/pull/1222#discussion_r453153111



##########
File path: ql/src/java/org/apache/hadoop/hive/ql/Driver.java
##########
@@ -139,205 +119,215 @@ public Driver(QueryState queryState, QueryInfo queryInfo, HiveTxnManager txnMana
     driverTxnHandler = new DriverTxnHandler(this, driverContext, driverState);
   }
 
-  /**
-   * Compile a new query, but potentially reset taskID counter.  Not resetting task counter
-   * is useful for generating re-entrant QL queries.
-   * @param command  The HiveQL query to compile
-   * @param resetTaskIds Resets taskID counter if true.
-   * @return 0 for ok
-   */
-  public int compile(String command, boolean resetTaskIds) {
-    try {
-      compile(command, resetTaskIds, false);
-      return 0;
-    } catch (CommandProcessorException cpr) {
-      return cpr.getErrorCode();
-    }
+  @Override
+  public Context getContext() {
+    return context;
   }
 
-  // deferClose indicates if the close/destroy should be deferred when the process has been
-  // interrupted, it should be set to true if the compile is called within another method like
-  // runInternal, which defers the close to the called in that method.
-  @VisibleForTesting
-  public void compile(String command, boolean resetTaskIds, boolean deferClose) throws CommandProcessorException {
-    preparForCompile(resetTaskIds);
-
-    Compiler compiler = new Compiler(context, driverContext, driverState);
-    QueryPlan plan = compiler.compile(command, deferClose);
-    driverContext.setPlan(plan);
-
-    compileFinished(deferClose);
+  @Override
+  public HiveConf getConf() {
+    return driverContext.getConf();
   }
 
-  private void compileFinished(boolean deferClose) {
-    if (DriverState.getDriverState().isAborted() && !deferClose) {
-      closeInProcess(true);
-    }
+  @Override
+  public CommandProcessorResponse run() throws CommandProcessorException {
+    return run(null, true);
   }
 
-  private void preparForCompile(boolean resetTaskIds) throws CommandProcessorException {
-    driverTxnHandler.createTxnManager();
-    DriverState.setDriverState(driverState);
-    prepareContext();
-    setQueryId();
+  @Override
+  public CommandProcessorResponse run(String command) throws CommandProcessorException {
+    return run(command, false);
+  }
 
-    if (resetTaskIds) {
-      TaskFactory.resetId();
+  private CommandProcessorResponse run(String command, boolean alreadyCompiled) throws CommandProcessorException {
+    try {
+      runInternal(command, alreadyCompiled);
+      return new CommandProcessorResponse(getSchema(), null);
+    } catch (CommandProcessorException cpe) {
+      processRunException(cpe);
+      throw cpe;
     }
   }
 
-  private void prepareContext() throws CommandProcessorException {
-    if (context != null && context.getExplainAnalyze() != AnalyzeState.RUNNING) {
-      // close the existing ctx etc before compiling a new query, but does not destroy driver
-      closeInProcess(false);
-    }
+  private void runInternal(String command, boolean alreadyCompiled) throws CommandProcessorException {
+    DriverState.setDriverState(driverState);
+    setInitialStateForRun(alreadyCompiled);
 
+    // a flag that helps to set the correct driver state in finally block by tracking if
+    // the method has been returned by an error or not.
+    boolean isFinishedWithError = true;
     try {
-      if (context == null) {
-        context = new Context(driverContext.getConf());
+      HiveDriverRunHookContext hookContext = new HiveDriverRunHookContextImpl(driverContext.getConf(),
+          alreadyCompiled ? context.getCmd() : command);
+      runPreDriverHooks(hookContext);
+
+      if (!alreadyCompiled) {
+        compileInternal(command, true);
+      } else {
+        driverContext.getPlan().setQueryStartTime(driverContext.getQueryDisplay().getQueryStartTime());
       }
-    } catch (IOException e) {
-      throw new CommandProcessorException(e);
-    }
 
-    context.setHiveTxnManager(driverContext.getTxnManager());
-    context.setStatsSource(driverContext.getStatsSource());
-    context.setHDFSCleanup(true);
+      // Reset the PerfLogger so that it doesn't retain any previous values.
+      // Any value from compilation phase can be obtained through the map set in queryDisplay during compilation.

Review comment:
       Shouldn't this be the first thing in the life cycle? Or minimally in this method? Previous code paths might already started to use the perf logger.




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