> On March 10, 2015, 8:01 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java, > > line 165 > > <https://reviews.apache.org/r/31871/diff/1/?file=889813#file889813line165> > > > > After my patch, QueryContext no longer needs to be closed -- so there's > > no possibility of leaking an exception here, because there can't be one. So > > I wouldn't worry about this for now -- you can leave it as-is if it's > > working for you, but beware of merging if this goes in after my patch.
After a discussion with Chris I will be making the following changes. I will make the QueryContext class properly return Exception, not IOException. I had written this originally to implement the Closable interface (which throws IOException) and ended up changing it to AutoClosable during the time I had rebased this on top of Chris' work for testing. This did not cause an issue with compilation or runtime, as IOExcption inherits from Exception, but it did unnecesarily create confusion as I had changed the signature from AutoCloseable. I will also add a return statement after the call to move to the FAILED state, this will prevent sending the message that was provided to the call to cleanup originally. As the moveToState method calls cleanup to send the failure message to the client, I will add a flag to QueryContext to make the close method idempotent as encouraged by the AutoClosable documentation. http://docs.oracle.com/javase/7/docs/api/java/lang/AutoCloseable.html - Jason ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31871/#review75958 ----------------------------------------------------------- On March 9, 2015, 10:07 p.m., Jason Altekruse wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31871/ > ----------------------------------------------------------- > > (Updated March 9, 2015, 10:07 p.m.) > > > Review request for drill and Jacques Nadeau. > > > Bugs: DRILL-2406 > https://issues.apache.org/jira/browse/DRILL-2406 > > > Repository: drill-git > > > Description > ------- > > To enable planning rules to take advantage of constant expressions in > queries, or to allow them to run queries against small datasets, such as > partition information we need to e able to evaluate expressions at planning > time. Expression evaluation in the regular expecuation path is relatively > expensive, as it invovles java code generation, compilation and JITing > expression trees. An interpreted expression system was added recently to > allow evaluating an expression without generating java code at runtime. This > patch exposes that work (after some refactorings in 2143 and the first part > of 2406) to the planning context for use in optimizer rules. > > > Diffs > ----- > > > exec/java-exec/src/main/java/org/apache/drill/exec/expr/fn/DrillFunctionRegistry.java > 00aaec6 > > exec/java-exec/src/main/java/org/apache/drill/exec/memory/TopLevelAllocator.java > 2a28bcb > exec/java-exec/src/main/java/org/apache/drill/exec/ops/BufferManager.java > PRE-CREATION > exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java > aa1dffd > exec/java-exec/src/main/java/org/apache/drill/exec/ops/QueryContext.java > c881432 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/logical/DrillReduceAggregatesRule.java > 93fff35 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/physical/visitor/InsertLocalExchangeVisitor.java > 907fcb1 > > exec/java-exec/src/main/java/org/apache/drill/exec/planner/sql/DrillSqlOperator.java > 6b54c43 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java > 409450f > > Diff: https://reviews.apache.org/r/31871/diff/ > > > Testing > ------- > > Cluster tests completed, waiting on unit tests. > > > Thanks, > > Jason Altekruse > >
