----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28686/#review70300 -----------------------------------------------------------
I would reiterate Mehant's review comment. The formatting, documentation and renaming changes need to go into another patch. With the number of changes in here, this patch is not reviewable. I could see substantial change only in QueryResultHandler which is reviewed. For the comments that have been added, it is preferable to actually add a comment than to add a TODO that suggests the comment to be added. Some of the TODO comments actually confused me. exec/java-exec/src/main/java/io/netty/buffer/DrillBuf.java <https://reviews.apache.org/r/28686/#comment115392> Because this (and other similar classes) need to access package private classes. exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java <https://reviews.apache.org/r/28686/#comment115393> This TODO is not very useful exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java <https://reviews.apache.org/r/28686/#comment115394> This TODO is not very useful. exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java <https://reviews.apache.org/r/28686/#comment115389> This switch statement sets isTerminalBatch to false for all cases. - Parth Chandra On Jan. 29, 2015, 5:47 a.m., Daniel Barclay wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28686/ > ----------------------------------------------------------- > > (Updated Jan. 29, 2015, 5:47 a.m.) > > > Review request for drill, Mehant Baid and Parth Chandra. > > > Bugs: DRILL-1735 > https://issues.apache.org/jira/browse/DRILL-1735 > > > Repository: drill-git > > > Description > ------- > > This change hooks in the previously written but unused DrillHandler so that a > local-mode JDBC connection shuts down its embedded DrillBit. > > It also fixes a chain of follow-on bugs (mostly resource-deallocation > omissions) exposed by the original bug fixes. (See the commit message for > the list of bugs/fixes and other changes.) > > > Diffs > ----- > > common/src/main/java/org/apache/drill/common/config/DrillConfig.java > 4c6c766 > exec/java-exec/src/main/codegen/templates/VariableLengthVectors.java > aa5b702 > exec/java-exec/src/main/java/io/netty/buffer/DrillBuf.java 8e9d395 > exec/java-exec/src/main/java/io/netty/buffer/FakeAllocator.java 3de0a75 > exec/java-exec/src/main/java/io/netty/buffer/PooledByteBufAllocatorL.java > c0de544 > exec/java-exec/src/main/java/io/netty/buffer/UnsafeDirectLittleEndian.java > dfdc114 > exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java > 04b955b > > exec/java-exec/src/main/java/org/apache/drill/exec/memory/BufferAllocator.java > 83d9d1e > exec/java-exec/src/main/java/org/apache/drill/exec/ops/FragmentContext.java > dc47f4e > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/base/FragmentRoot.java > 1721fcf > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/RootExec.java > a644c34 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScanBatch.java > d68a5b5 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java > d884200 > > exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractRecordBatch.java > 2bb29e5 > > exec/java-exec/src/main/java/org/apache/drill/exec/record/AbstractSingleRecordBatch.java > f895f47 > exec/java-exec/src/main/java/org/apache/drill/exec/record/RecordBatch.java > 0a8ece5 > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/BasicServer.java > c00df4e > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/RpcBus.java 96c9911 > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java > 9015a16 > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserResultsListener.java > 9f83a4f > > exec/java-exec/src/main/java/org/apache/drill/exec/server/BootStrapContext.java > 3da2ea9 > > exec/java-exec/src/main/java/org/apache/drill/exec/vector/accessor/AbstractSqlAccessor.java > 1605c7d > > exec/java-exec/src/main/java/org/apache/drill/exec/vector/accessor/SqlAccessor.java > b8480b4 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/CancelableQuery.java > 5b11943 > exec/java-exec/src/main/java/org/apache/drill/exec/work/WorkManager.java > 5bc3da1 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java > b33042b > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryManager.java > 2de3592 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/QueryStatus.java > 4e18da6 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/fragment/FragmentExecutor.java > 9ffe643 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/user/UserWorker.java > 854f474 > exec/java-exec/src/main/java/parquet/hadoop/CodecFactoryExposer.java > 4d107e4 > exec/java-exec/src/main/java/parquet/hadoop/ColumnChunkIncReadStore.java > 2a3124c > > exec/java-exec/src/main/java/parquet/hadoop/ColumnChunkPageWriteStoreExposer.java > 0e9dec0 > exec/java-exec/src/test/java/org/apache/drill/exec/ExecTest.java 0272b23 > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestComparisonFunctions.java > 609bc14 > exec/jdbc/src/main/java/org/apache/drill/jdbc/AvaticaDrillSqlAccessor.java > cf5829a > exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillAccessorList.java > 7594783 > exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillConnectionConfig.java > 584bbb4 > exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillConnectionImpl.java > 4c54780 > exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillCursor.java fbe611f > exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java 88a6c6d > exec/jdbc/src/main/java/org/apache/drill/jdbc/Driver.java 974e786 > > exec/jdbc/src/main/java/org/apache/drill/jdbc/InvalidCursorStateSqlException.java > PRE-CREATION > exec/jdbc/src/main/java/org/apache/drill/jdbc/JdbcApiSqlException.java > PRE-CREATION > exec/jdbc/src/test/java/org/apache/drill/jdbc/DrillResultSetTest.java > PRE-CREATION > exec/jdbc/src/test/java/org/apache/drill/jdbc/DriverTest.java PRE-CREATION > > exec/jdbc/src/test/java/org/apache/drill/jdbc/test/Bug1735ConnectionCloseTest.java > PRE-CREATION > > exec/jdbc/src/test/java/org/apache/drill/jdbc/test/Bug1735ResultSetCloseReleasesBuffersTest.java > PRE-CREATION > exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestJdbcDistQuery.java > b2f86ea > exec/jdbc/src/test/java/org/apache/drill/jdbc/test/TestViews.java 28fa4a7 > > Diff: https://reviews.apache.org/r/28686/diff/ > > > Testing > ------- > > Development/unit tests run with no errors. > > QA/cluster tests don't seem to fail any worse than they already do for > Drill's master branch ( :-( ): > > The Functional - Passing - new has failures for: > .../Functional/Passing/partition_pruning/dfs/parquet/plan/parquetselectStarFromPartition.q > .../Functional/Passing/partition_pruning/dfs/json/plan/jsonselectStarFromPartition.q > .../Functional/Passing/partition_pruning/dfs/hierarchical/plan/textSelectStartFromPartition.q > > The TPCH SF100 Parquet test suite has failures for: > - .../Advanced/Passing/tpch100/parquet/10.q > - .../Advanced/Passing/tpch100/parquet/01.q > > > Thanks, > > Daniel Barclay > >
