You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2020/02/09 06:45:20 UTC
[GitHub] [drill] paul-rogers opened a new pull request #1975: DRILL-7576:
Fail fast for operator errors
paul-rogers opened a new pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975
Converts operators to fail with a UserException rather than using
the STOP iterator status. The result is clearer error messages
and simpler code.
## Description
Drill has long had two ways to indicate operator failures:
* By throwing an unchecked exception, preferably `UserException`.
* By setting the failed status on the operator and returning the `STOP` iterator status.
Since the exception mechanism has been stable for several years, there is little reason to use the complex mechanism involved with the `STOP` status. Rather than write code to propagate `STOP` up the stack, we just let Java unwind the stack for us.
This PR converts all places where we returned `STOP` status to instead throw an exception. It also cleans up some lingering places where lower-level calls threw generic exceptions to higher-level code. This PR catches the error as close as possible to the source of the error to allow clearer, more focused error messages.
One consequence of using exceptions to report errors is that there is no longer any code to set the "failed" status for an operator. Mostly this doesn't matter; no code (except `STOP` handling) uses that status. Except in one place: the handy utility which dumps two batches on operator failure. That code relied on the failure status. Since most errors didn't set the status, the batch dump code did not actually work well.
Instead of having each operator catch the exception and mark itself failed. this PR uses a more general solution. It compares the stack trace in the exception with the list of operators to determine the leaf-most operator. This must be the operator which failed. This operator, and its parent, are not those dumped by the batch dump code.
## Documentation
This code is internal to Drill and is not user-visible.
## Testing
Added a new test for the stack trace analyzer. Reran all unit tests and adjusted those that needed changes.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376926556
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/TopN/PriorityQueueTemplate.java
##########
@@ -142,11 +143,7 @@ public void add(RecordBatchData batch) throws SchemaChangeException{
public void generate() {
Stopwatch watch = Stopwatch.createStarted();
final DrillBuf drillBuf = allocator.buffer(4 * queueSize);
- try {
- finalSv4 = new SelectionVector4(drillBuf, queueSize, 4000);
- } catch (SchemaChangeException e) {
- throw AbstractRecordBatch.schemaChangeException(e, "Priority Queue", logger);
- }
+ finalSv4 = new SelectionVector4(drillBuf, queueSize, 4000);
Review comment:
please convert magic number to constant.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376942403
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/WriterRecordBatch.java
##########
@@ -77,54 +80,53 @@ public BatchSchema getSchema() {
@Override
public IterOutcome innerNext() {
if (processed) {
-// cleanup();
// if the upstream record batch is already processed and next() is called by
// downstream then return NONE to indicate completion
return IterOutcome.NONE;
}
// process the complete upstream in one next() call
IterOutcome upstream;
- try {
- do {
- upstream = next(incoming);
-
- switch(upstream) {
- case STOP:
- return upstream;
-
- case NOT_YET:
+ do {
+ upstream = next(incoming);
+
+ switch(upstream) {
+ case STOP:
+ return upstream;
+
+ case NOT_YET:
+ break;
+ case NONE:
+ if (schema != null) {
+ // Schema is for the output batch schema which is setup in setupNewSchema(). Since the output
+ // schema is fixed ((Fragment(VARCHAR), Number of records written (BIGINT)) we should set it
+ // up even with 0 records for it to be reported back to the client.
break;
- case NONE:
- if (schema != null) {
- // Schema is for the output batch schema which is setup in setupNewSchema(). Since the output
- // schema is fixed ((Fragment(VARCHAR), Number of records written (BIGINT)) we should set it
- // up even with 0 records for it to be reported back to the client.
- break;
- }
-
- case OK_NEW_SCHEMA:
- setupNewSchema();
- // $FALL-THROUGH$
- case OK:
- counter += eventBasedRecordWriter.write(incoming.getRecordCount());
- logger.debug("Total records written so far: {}", counter);
+ }
- for(final VectorWrapper<?> v : incoming) {
- v.getValueVector().clear();
- }
- break;
-
- default:
- throw new UnsupportedOperationException();
- }
- } while(upstream != IterOutcome.NONE);
- } catch(IOException ex) {
- logger.error("Failure during query", ex);
- kill(false);
- context.getExecutorState().fail(ex);
- return IterOutcome.STOP;
- }
+ case OK_NEW_SCHEMA:
+ setupNewSchema();
+ // $FALL-THROUGH$
+ case OK:
+ try {
+ counter += eventBasedRecordWriter.write(incoming.getRecordCount());
+ } catch (IOException e) {
+ // TODO: Better handled inside the write() method.
Review comment:
can we avoid leaving new todos in the code?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376951117
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/filter/RuntimeFilterBatchCreator.java
##########
@@ -29,7 +29,9 @@
public class RuntimeFilterBatchCreator implements BatchCreator<RuntimeFilterPOP>{
Review comment:
```suggestion
public class RuntimeFilterBatchCreator implements BatchCreator<RuntimeFilterPOP> {
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376890243
##########
File path: common/src/main/java/org/apache/drill/common/exceptions/DrillRuntimeException.java
##########
@@ -17,8 +17,11 @@
*/
package org.apache.drill.common.exceptions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
public class DrillRuntimeException extends RuntimeException {
- private static final org.slf4j.Logger logger = org.slf4j.LoggerFactory.getLogger(DrillRuntimeException.class);
+ private static final Logger logger = LoggerFactory.getLogger(DrillRuntimeException.class);
Review comment:
Seems like the logger is unused and may be safely removed.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376947666
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/SpilledRecordBatch.java
##########
@@ -163,11 +162,12 @@ public IterOutcome next() {
container = vas.get();
}
} catch (IOException e) {
- lastOutcome = IterOutcome.STOP;
- throw UserException.dataReadError(e).addContext("Failed reading from a spill file").build(logger);
+ throw UserException.dataReadError(e)
+ .addContext("Failed reading from a spill file")
+ .build(logger);
} catch (Exception e) {
- lastOutcome = IterOutcome.STOP;
- throw e;
+ // TODO: Catch the error closer to the cause and create a better error message.
Review comment:
can we avoid adding new todos?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r378000995
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java
##########
@@ -87,6 +87,8 @@
* value is determined by where each record falls in the partition table. This
* column is used by PartitionSenderRootExec to determine which bucket to assign
* each record to.
+ * <p>
+ * This code is not used.
Review comment:
The idea is good, so I leave it in the code as a future reference.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376999100
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/orderedpartitioner/OrderedPartitionRecordBatch.java
##########
@@ -87,6 +87,8 @@
* value is determined by where each record falls in the partition table. This
* column is used by PartitionSenderRootExec to determine which bucket to assign
* each record to.
+ * <p>
+ * This code is not used.
Review comment:
if the class is never used why don't just delete it?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376924072
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/StatisticsWriterRecordBatch.java
##########
@@ -154,11 +162,18 @@ private void addOutputContainerData() {
container.setRecordCount(1);
}
- protected void setupNewSchema() throws IOException {
+ protected void setupNewSchema() {
try {
// update the schema in RecordWriter
stats.startSetup();
- recordWriter.updateSchema(incoming);
+ try {
+ recordWriter.updateSchema(incoming);
+ } catch (IOException e) {
+ // TODO: This is better handled inside updateSchema()
Review comment:
I would like to ask for fixing it, thank you in advance.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376942618
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/WriterRecordBatch.java
##########
@@ -152,11 +154,18 @@ private void addOutputContainerData() {
container.setRecordCount(1);
}
- protected void setupNewSchema() throws IOException {
+ protected void setupNewSchema() {
try {
// update the schema in RecordWriter
stats.startSetup();
- recordWriter.updateSchema(incoming);
+ try {
+ recordWriter.updateSchema(incoming);
+ } catch (IOException e) {
+ // TODO: This is better handled inside updateSchema()
Review comment:
same question as above
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376945843
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggBatch.java
##########
@@ -340,16 +340,12 @@ public IterOutcome innerNext() {
}
case UPDATE_AGGREGATOR:
- context.getExecutorState().fail(UserException.unsupportedError()
+ throw UserException.unsupportedError()
.message(SchemaChangeException.schemaChanged(
"Hash aggregate does not support schema change",
incomingSchema,
incoming.getSchema()).getMessage())
- .build(logger));
- close();
- killIncoming(false);
- firstBatch = false;
- return IterOutcome.STOP;
+ .build(logger);
Review comment:
could you please explain why ```close()``` and ```killIncoming(false)``` invocations were removed for the case?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376954034
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/filter/RuntimeFilterRecordBatch.java
##########
@@ -196,20 +194,24 @@ private void setupHashHelper() {
ValueVectorReadExpression toHashFieldExp = new ValueVectorReadExpression(typedFieldId);
hashFieldExps.add(toHashFieldExp);
}
- hash64 = hashHelper.getHash64(hashFieldExps.toArray(new LogicalExpression[hashFieldExps.size()]), typedFieldIds.toArray(new TypedFieldId[typedFieldIds.size()]));
+ hash64 = hashHelper.getHash64(hashFieldExps.toArray(
+ new LogicalExpression[hashFieldExps.size()]),
+ typedFieldIds.toArray(new TypedFieldId[typedFieldIds.size()]));
} catch (Exception e) {
throw UserException.internalError(e).build(logger);
}
}
}
/**
- * If RuntimeFilter is available then applies the filter condition on the incoming batch records and creates an SV2
- * to store indexes which passes the filter condition. In case when RuntimeFilter is not available it just pass
+ * If RuntimeFilter is available then applies the filter condition on the
+ * incoming batch records and creates an SV2 to store indexes which passes the
+ * filter condition. In case when RuntimeFilter is not available it just pass
* through all the records from incoming batch to downstream.
+ *
* @throws SchemaChangeException
Review comment:
```suggestion
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376903426
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java
##########
@@ -184,4 +179,38 @@ public void close() throws Exception {
}
}
}
+
+ /**
+ * Given a list of operators and a stack trace, walks the stack trace and
+ * the operator list to find the leaf-most operator, which is the one
+ * that was active when the exception was thrown. Handle the cases in
+ * which no operator was active, each operator had multiple methods on
+ * the stack, or the exception was thrown in some class called by
+ * the operator.
+ * <p>
+ * Not all operators leave a mark in the trace. In particular if a the
+ * call stack is only through base-class methods, then we have no way to
+ * know the actual class during the call. This is OK because the leaf
+ * methods are just pass-through operations, they are unlikely to fail.
+ *
+ * @param <T> the type of the operator. Parameterized to allow easier
+ * testing
+ * @param dag the list of operators from root-most to leaf-most
+ * @param e the exception thrown somewhere in the operator tree
+ * @return the leaf-most operator, if any
+ */
+ public static <T> T findLeaf(List<T> dag, Throwable e) {
+ StackTraceElement[] trace = e.getStackTrace();
Review comment:
Minor comment: here we could collect a ```Set``` of class name strings ignoring those names which don't start with ```org.apache.drill``` package name. Like:
```java
Set<String> drillStackTrace = Stream.of(trace)
.map(StackTraceElement::getClassName)
.filter(cn -> cn.startsWith(DRILL_PACKAGE_NAME))
.collect(Collectors.toSet());
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] asfgit closed pull request #1975: DRILL-7576: Fail fast for
operator errors
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376908070
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/RootExec.java
##########
@@ -20,34 +20,38 @@
import org.apache.drill.exec.proto.ExecProtos.FragmentHandle;
/**
- * <h2>Functionality</h2>
- * <p>
- * A FragmentRoot is a node which is the last processing node in a query plan. FragmentTerminals include Exchange
- * output nodes and storage nodes. They are there driving force behind the completion of a query.
- * </p>
- * <h2>Assumptions</h2>
- * <p>
- * All implementations of {@link RootExec} assume that all their methods are called by the same thread.
+ * Node which is the last processing node in a query plan. FragmentTerminals
+ * include Exchange output nodes and storage nodes. They are there driving force
+ * behind the completion of a query.
* </p>
+ * Assumes that all implementations of {@link RootExec} assume that all their
+ * methods are called by the same thread.
*/
public interface RootExec extends AutoCloseable {
+
/**
* Do the next batch of work.
- * @return Whether or not additional batches of work are necessary. False means that this fragment is done.
+ *
+ * @return Whether or not additional batches of work are necessary. False
+ * means that this fragment is done.
*/
boolean next();
/**
- * Inform sender that receiving fragment is finished and doesn't need any more data. This can be called multiple
- * times (once for each downstream receiver). If all receivers are finished then a subsequent call to {@link #next()}
- * will return false.
- * @param handle The handle pointing to the downstream receiver that does not need anymore data.
+ * Inform sender that receiving fragment is finished and doesn't need any more
+ * data. This can be called multiple times (once for each downstream
+ * receiver). If all receivers are finished then a subsequent call to
+ * {@link #next()} will return false.
+ *
+ * @param handle
+ * The handle pointing to the downstream receiver that does not need
+ * anymore data.
*/
void receivingFragmentFinished(FragmentHandle handle);
/**
- * Dump failed batches' state preceded by its parent's state to logs. Invoked when there is a
- * failure during fragment execution.
+ * Dump failed batches' state preceded by its parent's state to logs. Invoked
+ * when there is a failure during fragment execution.
Review comment:
Please add ```@param``` section to the JavaDoc describing where the trowable came from.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376912149
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java
##########
@@ -212,13 +213,24 @@ private boolean shouldContinueAfterNoRecords() throws Exception {
}
return true;
} else { // Regular scan
- currentReader.close();
- currentReader = null;
+ closeCurrentReader();
return true; // In regular case, we always continue the iteration, if no more reader, we will break out at the head of loop
}
}
- private IterOutcome internalNext() throws Exception {
+ private void closeCurrentReader() {
+ try {
+ if (currentReader != null) {
Review comment:
I would suggest putting whole ```try-catch-finally``` inside the if, or using our ```Autocloseables``` like:
```java
AutoCloseables.closeSilently(currentReader);
currentReader = null;
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r378086624
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java
##########
@@ -184,4 +179,38 @@ public void close() throws Exception {
}
}
}
+
+ /**
+ * Given a list of operators and a stack trace, walks the stack trace and
+ * the operator list to find the leaf-most operator, which is the one
+ * that was active when the exception was thrown. Handle the cases in
+ * which no operator was active, each operator had multiple methods on
+ * the stack, or the exception was thrown in some class called by
+ * the operator.
+ * <p>
+ * Not all operators leave a mark in the trace. In particular if a the
+ * call stack is only through base-class methods, then we have no way to
+ * know the actual class during the call. This is OK because the leaf
+ * methods are just pass-through operations, they are unlikely to fail.
+ *
+ * @param <T> the type of the operator. Parameterized to allow easier
+ * testing
+ * @param dag the list of operators from root-most to leaf-most
+ * @param e the exception thrown somewhere in the operator tree
+ * @return the leaf-most operator, if any
+ */
+ public static <T> T findLeaf(List<T> dag, Throwable e) {
+ StackTraceElement[] trace = e.getStackTrace();
Review comment:
I see the order is important for the outer loop
```java
for (int i = dag.size() - 1; i >= 0; i--) {
...
}
```
but in an inner loop, you're iterating for each stack trace element so the suggested ```Set<String>``` won't break your algorithm. Though I don't force the change since we know that operators and stack trace collections are small.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r377999682
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/SpilledRecordBatch.java
##########
@@ -163,11 +162,12 @@ public IterOutcome next() {
container = vas.get();
}
} catch (IOException e) {
- lastOutcome = IterOutcome.STOP;
- throw UserException.dataReadError(e).addContext("Failed reading from a spill file").build(logger);
+ throw UserException.dataReadError(e)
+ .addContext("Failed reading from a spill file")
+ .build(logger);
} catch (Exception e) {
- lastOutcome = IterOutcome.STOP;
- throw e;
+ // TODO: Catch the error closer to the cause and create a better error message.
Review comment:
Cleaned up some. But others are really beyond the scope of this PR as they require extra investigation. The "TODO" helps us remember to do that work. Else, any one PR gets too large if we have to fix all Drill's challenges in a single go.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r377999381
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/aggregate/HashAggBatch.java
##########
@@ -340,16 +340,12 @@ public IterOutcome innerNext() {
}
case UPDATE_AGGREGATOR:
- context.getExecutorState().fail(UserException.unsupportedError()
+ throw UserException.unsupportedError()
.message(SchemaChangeException.schemaChanged(
"Hash aggregate does not support schema change",
incomingSchema,
incoming.getSchema()).getMessage())
- .build(logger));
- close();
- killIncoming(false);
- firstBatch = false;
- return IterOutcome.STOP;
+ .build(logger);
Review comment:
We are throwing an exception. In all such cases, the first thing the fragment executor does is close all the active operators. So, no need for us to tell upstream that we're failing, or for us to close here. This is, in fact, an ambiguity that has lead to several bugs when we've closed things twice in a way that triggers errors.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r377003326
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/FrameSupportTemplate.java
##########
@@ -175,7 +177,7 @@ private int processROWS(int row) throws DrillException {
return row;
}
- private int processRANGE(int row) throws DrillException {
+ private int processRANGE(int row) throws SchemaChangeException {
Review comment:
```suggestion
private int processRange(int row) throws SchemaChangeException {
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376893361
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/PhysicalOperatorUtil.java
##########
@@ -47,6 +47,7 @@ private PhysicalOperatorUtil() { }
/**
* Helper method to create a list of {@code MinorFragmentEndpoint} instances from a
+ * Helper method to create a list of MinorFragmentEndpoint instances from a
Review comment:
accidental change?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376951049
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/filter/RuntimeFilterBatchCreator.java
##########
@@ -29,7 +29,9 @@
public class RuntimeFilterBatchCreator implements BatchCreator<RuntimeFilterPOP>{
@Override
- public CloseableRecordBatch getBatch(ExecutorFragmentContext context, RuntimeFilterPOP config, List<RecordBatch> children) throws ExecutionSetupException {
+ public CloseableRecordBatch getBatch(ExecutorFragmentContext context,
+ RuntimeFilterPOP config, List<RecordBatch> children)
+ throws ExecutionSetupException {
Review comment:
```java
public CloseableRecordBatch getBatch(ExecutorFragmentContext context, RuntimeFilterPOP config,
List<RecordBatch> children) throws ExecutionSetupException {
```
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376923043
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/StatisticsWriterRecordBatch.java
##########
@@ -91,41 +93,47 @@ public IterOutcome innerNext() {
}
// process the complete upstream in one next() call
IterOutcome upstream;
- try {
- do {
- upstream = next(incoming);
-
- switch(upstream) {
- case STOP:
- return upstream;
-
- case NOT_YET:
- case NONE:
- break;
-
- case OK_NEW_SCHEMA:
- setupNewSchema();
- // $FALL-THROUGH$
- case OK:
+ do {
+ upstream = next(incoming);
+
+ switch(upstream) {
+ case STOP:
+ return upstream;
+
+ case NOT_YET:
+ case NONE:
+ break;
+
+ case OK_NEW_SCHEMA:
+ setupNewSchema();
+ // $FALL-THROUGH$
+ case OK:
+ try {
counter += statsRecordWriterImpl.writeStatistics(incoming.getRecordCount());
- logger.debug("Total records written so far: {}", counter);
-
- for(final VectorWrapper<?> v : incoming) {
- v.getValueVector().clear();
- }
- break;
-
- default:
- throw new UnsupportedOperationException();
- }
- } while(upstream != IterOutcome.NONE);
- // Flush blocking writers now
+ } catch (IOException e) {
+ // TODO: Better handled inside the write() method.
Review comment:
such todos usually live forever in the code, could you please improve the ```StatisticsRecordWriterImpl.writeStatistics``` in this PR?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r377985719
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/BaseRootExec.java
##########
@@ -184,4 +179,38 @@ public void close() throws Exception {
}
}
}
+
+ /**
+ * Given a list of operators and a stack trace, walks the stack trace and
+ * the operator list to find the leaf-most operator, which is the one
+ * that was active when the exception was thrown. Handle the cases in
+ * which no operator was active, each operator had multiple methods on
+ * the stack, or the exception was thrown in some class called by
+ * the operator.
+ * <p>
+ * Not all operators leave a mark in the trace. In particular if a the
+ * call stack is only through base-class methods, then we have no way to
+ * know the actual class during the call. This is OK because the leaf
+ * methods are just pass-through operations, they are unlikely to fail.
+ *
+ * @param <T> the type of the operator. Parameterized to allow easier
+ * testing
+ * @param dag the list of operators from root-most to leaf-most
+ * @param e the exception thrown somewhere in the operator tree
+ * @return the leaf-most operator, if any
+ */
+ public static <T> T findLeaf(List<T> dag, Throwable e) {
+ StackTraceElement[] trace = e.getStackTrace();
Review comment:
Here order is important. Yes, we could pre-process the list to remove all non-Drill methods. Not sure the time savings is worth the code complexity given that this code occurs only in error conditions, and only when enabled.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r376919500
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java
##########
@@ -83,47 +83,47 @@ public boolean innerNext() {
IterOutcome outcome = next(incoming);
logger.trace("Screen Outcome {}", outcome);
switch (outcome) {
- case STOP:
- return false;
- case NONE:
- if (firstBatch) {
- // this is the only data message sent to the client and may contain the schema
- QueryWritableBatch batch;
- QueryData header = QueryData.newBuilder()
- .setQueryId(context.getHandle().getQueryId())
- .setRowCount(0)
- .setDef(RecordBatchDef.getDefaultInstance())
- .build();
- batch = new QueryWritableBatch(header);
+ case STOP:
+ return false;
+ case NONE:
+ if (firstBatch) {
+ // this is the only data message sent to the client and may contain the schema
+ QueryWritableBatch batch;
+ QueryData header = QueryData.newBuilder()
+ .setQueryId(context.getHandle().getQueryId())
+ .setRowCount(0)
+ .setDef(RecordBatchDef.getDefaultInstance())
+ .build();
+ batch = new QueryWritableBatch(header);
+
+ stats.startWait();
+ try {
+ userConnection.sendData(batch);
+ } finally {
+ stats.stopWait();
+ }
+ firstBatch = false; // we don't really need to set this. But who knows!
+ }
+ return false;
+ case OK_NEW_SCHEMA:
+ materializer = new VectorRecordMaterializer(context, oContext, incoming);
+ //$FALL-THROUGH$
+ case OK:
+ injector.injectPause(context.getExecutionControls(), "sending-data", logger);
+ final QueryWritableBatch batch = materializer.convertNext();
+ updateStats(batch);
stats.startWait();
try {
userConnection.sendData(batch);
} finally {
stats.stopWait();
}
- firstBatch = false; // we don't really need to set this. But who knows!
- }
-
- return false;
- case OK_NEW_SCHEMA:
- materializer = new VectorRecordMaterializer(context, oContext, incoming);
- //$FALL-THROUGH$
- case OK:
- injector.injectPause(context.getExecutionControls(), "sending-data", logger);
- final QueryWritableBatch batch = materializer.convertNext();
- updateStats(batch);
- stats.startWait();
- try {
- userConnection.sendData(batch);
- } finally {
- stats.stopWait();
- }
- firstBatch = false;
-
- return true;
- default:
- throw new UnsupportedOperationException();
+ firstBatch = false;
+
+ return true;
+ default:
+ throw new UnsupportedOperationException();
Review comment:
Here would be nice to add message containing ```outcome``` which caused the exception.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] paul-rogers commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
paul-rogers commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r378001509
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/FrameSupportTemplate.java
##########
@@ -156,7 +158,7 @@ private int processPartition(final int currentRow) throws DrillException {
}
}
- private int processROWS(int row) throws DrillException {
+ private int processROWS(int row) throws SchemaChangeException {
Review comment:
I think the author refers to the SQL `ROWS` keyword. There is also a `processRANGE`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [drill] ihuzenko commented on a change in pull request #1975:
DRILL-7576: Fail fast for operator errors
Posted by GitBox <gi...@apache.org>.
ihuzenko commented on a change in pull request #1975: DRILL-7576: Fail fast for operator errors
URL: https://github.com/apache/drill/pull/1975#discussion_r377003222
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/window/FrameSupportTemplate.java
##########
@@ -156,7 +158,7 @@ private int processPartition(final int currentRow) throws DrillException {
}
}
- private int processROWS(int row) throws DrillException {
+ private int processROWS(int row) throws SchemaChangeException {
Review comment:
```suggestion
private int processRows(int row) throws SchemaChangeException {
```
----------------------------------------------------------------
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
With regards,
Apache Git Services