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 2020/09/21 00:08:44 UTC
[GitHub] [hive] miklosgergely opened a new pull request #1512: HIVE-24184: Re-order methods in Driver (Miklos Gergely)
miklosgergely opened a new pull request #1512:
URL: https://github.com/apache/hive/pull/1512
### What changes were proposed in this pull request?
Driver is still a huge class, with a lot of methods. They are not representing the order of the process done by the Driver (compilation, execution, result providing, closing). Also the constructors are not at the beginning of the class. All of these make the class harder to read. By re-ordering them it would be easier.
### Why are the changes needed?
To make the Driver class cleaner, thus easier to read/understand.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
All the tests are still passing.
----------------------------------------------------------------
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
[GitHub] [hive] miklosgergely commented on a change in pull request #1512: HIVE-24184: Re-order methods in Driver (Miklos Gergely)
Posted by GitBox <gi...@apache.org>.
miklosgergely commented on a change in pull request #1512:
URL: https://github.com/apache/hive/pull/1512#discussion_r504039229
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/Driver.java
##########
@@ -588,15 +353,280 @@ private void runInternal(String command, boolean alreadyCompiled) throws Command
SessionState.getPerfLogger().cleanupPerfLogMetrics();
}
- @Override
- public boolean isFetchingTable() {
- return driverContext.getFetchTask() != null;
+ public void lockAndRespond() throws CommandProcessorException {
+ // Assumes the query has already been compiled
+ if (driverContext.getPlan() == null) {
+ throw new IllegalStateException(
+ "No previously compiled query for driver - queryId=" + driverContext.getQueryState().getQueryId());
+ }
+
+ try {
+ driverTxnHandler.acquireLocksIfNeeded();
+ } catch (CommandProcessorException cpe) {
+ driverTxnHandler.rollback(cpe);
+ throw cpe;
+ }
}
- @SuppressWarnings({ "unchecked", "rawtypes" })
@Override
- public boolean getResults(List res) throws IOException {
- if (driverState.isDestroyed() || driverState.isClosed()) {
+ public CommandProcessorResponse compileAndRespond(String command) throws CommandProcessorException {
+ return compileAndRespond(command, false);
+ }
+
+ public CommandProcessorResponse compileAndRespond(String command, boolean cleanupTxnList)
+ throws CommandProcessorException {
+ try {
+ compileInternal(command, false);
+ return new CommandProcessorResponse(getSchema(), null);
+ } catch (CommandProcessorException cpe) {
+ throw cpe;
+ } finally {
+ if (cleanupTxnList) {
+ // Valid txn list might be generated for a query compiled using this command, thus we need to reset it
+ driverTxnHandler.cleanupTxnList();
+ }
+ }
+ }
+
+ private void compileInternal(String command, boolean deferClose) throws CommandProcessorException {
+ Metrics metrics = MetricsFactory.getInstance();
+ if (metrics != null) {
+ metrics.incrementCounter(MetricsConstant.WAITING_COMPILE_OPS, 1);
+ }
+
+ PerfLogger perfLogger = SessionState.getPerfLogger(true);
+ perfLogger.perfLogBegin(CLASS_NAME, PerfLogger.WAIT_COMPILE);
+
+ try (CompileLock compileLock = CompileLockFactory.newInstance(driverContext.getConf(), command)) {
+ boolean success = compileLock.tryAcquire();
+
+ perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.WAIT_COMPILE);
+
+ if (metrics != null) {
+ metrics.decrementCounter(MetricsConstant.WAITING_COMPILE_OPS, 1);
+ }
+ if (!success) {
+ String errorMessage = ErrorMsg.COMPILE_LOCK_TIMED_OUT.getErrorCodedMsg();
+ throw DriverUtils.createProcessorException(driverContext, ErrorMsg.COMPILE_LOCK_TIMED_OUT.getErrorCode(),
+ errorMessage, null, null);
+ }
+
+ try {
+ compile(command, true, deferClose);
+ } catch (CommandProcessorException cpe) {
+ try {
+ driverTxnHandler.endTransactionAndCleanup(false);
+ } catch (LockException e) {
+ LOG.warn("Exception in releasing locks. " + StringUtils.stringifyException(e));
+ }
+ throw cpe;
+ }
+ }
+ //Save compile-time PerfLogging for WebUI.
+ //Execution-time Perf logs are done by either another thread's PerfLogger
+ //or a reset PerfLogger.
+ driverContext.getQueryDisplay().setPerfLogStarts(QueryDisplay.Phase.COMPILATION, perfLogger.getStartTimes());
+ driverContext.getQueryDisplay().setPerfLogEnds(QueryDisplay.Phase.COMPILATION, perfLogger.getEndTimes());
+ }
+
+ /**
+ * Compiles a new HQL command, but potentially resets 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();
+ }
+ }
+
+ /**
+ * Compiles an HQL command, creates an execution plan for it.
+ *
+ * @param 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.
+ * @param resetTaskIds Resets taskID counter if true.
+ */
+ @VisibleForTesting
+ public void compile(String command, boolean resetTaskIds, boolean deferClose) throws CommandProcessorException {
Review comment:
Thank you @belugabehr, I've fixed these.
----------------------------------------------------------------
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
[GitHub] [hive] belugabehr commented on pull request #1512: HIVE-24184: Re-order methods in Driver (Miklos Gergely)
Posted by GitBox <gi...@apache.org>.
belugabehr commented on pull request #1512:
URL: https://github.com/apache/hive/pull/1512#issuecomment-707816599
LGTM pending 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.
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] belugabehr commented on a change in pull request #1512: HIVE-24184: Re-order methods in Driver (Miklos Gergely)
Posted by GitBox <gi...@apache.org>.
belugabehr commented on a change in pull request #1512:
URL: https://github.com/apache/hive/pull/1512#discussion_r504025808
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/Driver.java
##########
@@ -588,15 +353,280 @@ private void runInternal(String command, boolean alreadyCompiled) throws Command
SessionState.getPerfLogger().cleanupPerfLogMetrics();
}
- @Override
- public boolean isFetchingTable() {
- return driverContext.getFetchTask() != null;
+ public void lockAndRespond() throws CommandProcessorException {
+ // Assumes the query has already been compiled
+ if (driverContext.getPlan() == null) {
+ throw new IllegalStateException(
+ "No previously compiled query for driver - queryId=" + driverContext.getQueryState().getQueryId());
+ }
+
+ try {
+ driverTxnHandler.acquireLocksIfNeeded();
+ } catch (CommandProcessorException cpe) {
+ driverTxnHandler.rollback(cpe);
+ throw cpe;
+ }
}
- @SuppressWarnings({ "unchecked", "rawtypes" })
@Override
- public boolean getResults(List res) throws IOException {
- if (driverState.isDestroyed() || driverState.isClosed()) {
+ public CommandProcessorResponse compileAndRespond(String command) throws CommandProcessorException {
+ return compileAndRespond(command, false);
+ }
+
+ public CommandProcessorResponse compileAndRespond(String command, boolean cleanupTxnList)
+ throws CommandProcessorException {
+ try {
+ compileInternal(command, false);
+ return new CommandProcessorResponse(getSchema(), null);
+ } catch (CommandProcessorException cpe) {
+ throw cpe;
+ } finally {
+ if (cleanupTxnList) {
+ // Valid txn list might be generated for a query compiled using this command, thus we need to reset it
+ driverTxnHandler.cleanupTxnList();
+ }
+ }
+ }
+
+ private void compileInternal(String command, boolean deferClose) throws CommandProcessorException {
+ Metrics metrics = MetricsFactory.getInstance();
+ if (metrics != null) {
+ metrics.incrementCounter(MetricsConstant.WAITING_COMPILE_OPS, 1);
+ }
+
+ PerfLogger perfLogger = SessionState.getPerfLogger(true);
+ perfLogger.perfLogBegin(CLASS_NAME, PerfLogger.WAIT_COMPILE);
+
+ try (CompileLock compileLock = CompileLockFactory.newInstance(driverContext.getConf(), command)) {
+ boolean success = compileLock.tryAcquire();
+
+ perfLogger.perfLogEnd(CLASS_NAME, PerfLogger.WAIT_COMPILE);
+
+ if (metrics != null) {
+ metrics.decrementCounter(MetricsConstant.WAITING_COMPILE_OPS, 1);
+ }
+ if (!success) {
+ String errorMessage = ErrorMsg.COMPILE_LOCK_TIMED_OUT.getErrorCodedMsg();
+ throw DriverUtils.createProcessorException(driverContext, ErrorMsg.COMPILE_LOCK_TIMED_OUT.getErrorCode(),
+ errorMessage, null, null);
+ }
+
+ try {
+ compile(command, true, deferClose);
+ } catch (CommandProcessorException cpe) {
+ try {
+ driverTxnHandler.endTransactionAndCleanup(false);
+ } catch (LockException e) {
+ LOG.warn("Exception in releasing locks. " + StringUtils.stringifyException(e));
+ }
+ throw cpe;
+ }
+ }
+ //Save compile-time PerfLogging for WebUI.
+ //Execution-time Perf logs are done by either another thread's PerfLogger
+ //or a reset PerfLogger.
+ driverContext.getQueryDisplay().setPerfLogStarts(QueryDisplay.Phase.COMPILATION, perfLogger.getStartTimes());
+ driverContext.getQueryDisplay().setPerfLogEnds(QueryDisplay.Phase.COMPILATION, perfLogger.getEndTimes());
+ }
+
+ /**
+ * Compiles a new HQL command, but potentially resets 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();
+ }
+ }
+
+ /**
+ * Compiles an HQL command, creates an execution plan for it.
+ *
+ * @param 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.
+ * @param resetTaskIds Resets taskID counter if true.
+ */
+ @VisibleForTesting
+ public void compile(String command, boolean resetTaskIds, boolean deferClose) throws CommandProcessorException {
Review comment:
Please change order of `@param` to match arguments and also add one for 'command' argument.
----------------------------------------------------------------
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
[GitHub] [hive] miklosgergely merged pull request #1512: HIVE-24184: Re-order methods in Driver (Miklos Gergely)
Posted by GitBox <gi...@apache.org>.
miklosgergely merged pull request #1512:
URL: https://github.com/apache/hive/pull/1512
----------------------------------------------------------------
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