----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32468/#review78086 -----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java <https://reviews.apache.org/r/32468/#comment126499> rest.getHeader() can be called once and reused here and in the try below. exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java <https://reviews.apache.org/r/32468/#comment127014> Please rename to "newUserResultsListener," or "createUserResultsListener." exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java <https://reviews.apache.org/r/32468/#comment127018> I don't know about the TODO, but it is cheaper to test for empty strings with isEmpty(). http://stackoverflow.com/questions/3321526/should-i-use-string-isempty-or-equalsstring exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java <https://reviews.apache.org/r/32468/#comment127023> How come output needs to be protected, but throttle doesn't? exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserResultsListener.java <https://reviews.apache.org/r/32468/#comment127025> I'm a little surprised this doesn't include the final query status in some form (the new QueryResult?). exec/java-exec/src/test/java/org/apache/drill/SingleRowListener.java <https://reviews.apache.org/r/32468/#comment127034> I'll bet that cleanup() can throw, so to avoid this causing hangs, perhaps it should be wrapped with a try-finally here. exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java <https://reviews.apache.org/r/32468/#comment127038> It doesn't look like this is called from anywhere else, so can we make it private? exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java <https://reviews.apache.org/r/32468/#comment127040> Just remove these comments. exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java <https://reviews.apache.org/r/32468/#comment127041> Why did you leave it here then? protocol/src/main/protobuf/UserBitShared.proto <https://reviews.apache.org/r/32468/#comment127046> In order to deal with the new suppressed exceptions, won't we need to add a "repeated ExceptionWrapper suppressed = 5" to this? protocol/src/main/protobuf/UserBitShared.proto <https://reviews.apache.org/r/32468/#comment127044> Re the removal of submission_time: are we now saying that the only source of this information will be to inspect the query's profile? I'm ok with that claim, I just wanted to be sure it's a conscious decision, and that this information is available from there. - Chris Westin On March 26, 2015, 6:20 p.m., abdelhakim deneche wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32468/ > ----------------------------------------------------------- > > (Updated March 26, 2015, 6:20 p.m.) > > > Review request for drill, Chris Westin, Daniel Barclay, and Jacques Nadeau. > > > Bugs: DRILL-2498 > https://issues.apache.org/jira/browse/DRILL-2498 > > > Repository: drill-git > > > Description > ------- > > INITIAL PATCH (looking for feedback) > > - removed **last chunk** from *QueryResult* > - separate **QueryResult** and **QueryData**, update RPC layer to handle this > separation > . **QueryResultHandler** handles data and result messages separately > . *UserClient* calls *QueryResultHandler.resultArrived()* / *dataArrived()* > depending on the received message > . bumped up **RPC_VERSION** in *UserRpcConfig* > . *UserServer* has separate *sendData()* / *sendResult()* > - updated **UserResultHandler** interface to handle data messages differently > from result messages: > . *dataArrived(QueryResultBatch result, ConnectionThrottle throttle)* is > called when a data batch is received > . *queryCompleted()* is called when a terminal (completed/canceled) message > is received > - the *Foreman* only sends QueryResult to the client > - In case of an error, The *ScreenRoot* doesn't send the error to the client. > The Foreman will send a FAILED *QueryResult* instead > - in case of a data batch, the *ScreenRoot* sends a *QueryData* to the client > - *QueryWritableBatch* and *QueryResultBatch* use *QueryData* instead of > *QueryResult* > - *VectorRecordMaterializer* builds a *QueryData* instead of *QueryResult* > - updated classes that implement *UserResultHandlers*: > . DrillClient.ListHoldingResultsListener > . PrintingResultsListener > . QueryWrapper.Listener > . BaseTestQuery.SilentListener > . SingleRowListener > . ParquetResultListener > . TestParquetPhysicalPlan > . DrillResultSet.ResultsListener > - updated several unit tests that were expecting an extra **last chunk** batch > . TestDateFunctions > . TestMultiInputAdd > . TestCastVarCharToBigInt > . TestDateTypes > - contains **DRILL-2521** (revert from proto 2.6.0 to 2.5.0) > will be removed from the final patch > - code cleanup: making loggers private, removing unused imports > > > Diffs > ----- > > exec/java-exec/src/main/java/org/apache/drill/exec/client/DrillClient.java > 6d4c86c > > exec/java-exec/src/main/java/org/apache/drill/exec/client/PrintingResultsListener.java > 926e703 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/ScreenCreator.java > 404c453 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/materialize/QueryWritableBatch.java > 2a59e22 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/materialize/RecordMaterializer.java > 221fc34 > > exec/java-exec/src/main/java/org/apache/drill/exec/physical/impl/materialize/VectorRecordMaterializer.java > cc1b3bf > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultBatch.java > ab4c9ef > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java > c05b127 > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserClient.java > 925154d > > 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/rpc/user/UserRpcConfig.java > 908d304 > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserServer.java > c76d324 > > exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/QueryWrapper.java > 8996a69 > > exec/java-exec/src/main/java/org/apache/drill/exec/work/foreman/Foreman.java > 285b75a > exec/java-exec/src/test/java/org/apache/drill/BaseTestQuery.java 64cf2ec > exec/java-exec/src/test/java/org/apache/drill/SingleRowListener.java > 07cb833 > > exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestDateFunctions.java > 67aa4dd > > exec/java-exec/src/test/java/org/apache/drill/exec/fn/impl/TestMultiInputAdd.java > 65848eb > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestCastVarCharToBigInt.java > 84ac8cf > > exec/java-exec/src/test/java/org/apache/drill/exec/physical/impl/TestConvertFunctions.java > fc4c0cc > > exec/java-exec/src/test/java/org/apache/drill/exec/record/vector/TestDateTypes.java > 4cc82e4 > > exec/java-exec/src/test/java/org/apache/drill/exec/server/TestDrillbitResilience.java > 9bc0552 > > exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/ParquetResultListener.java > 52d5086 > > exec/java-exec/src/test/java/org/apache/drill/exec/store/parquet/TestParquetPhysicalPlan.java > 6cb412c > > exec/java-exec/src/test/java/org/apache/drill/exec/vector/complex/writer/TestJsonReader.java > fe86192 > exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java 0ce33f4 > protocol/pom.xml f7d907d > protocol/src/main/java/org/apache/drill/common/types/TypeProtos.java > 67beed1 > protocol/src/main/java/org/apache/drill/exec/proto/BitControl.java 2502701 > protocol/src/main/java/org/apache/drill/exec/proto/BitData.java b4a2f19 > protocol/src/main/java/org/apache/drill/exec/proto/CoordinationProtos.java > 45321ea > protocol/src/main/java/org/apache/drill/exec/proto/ExecProtos.java ca4f294 > protocol/src/main/java/org/apache/drill/exec/proto/GeneralRPCProtos.java > 32a63e5 > protocol/src/main/java/org/apache/drill/exec/proto/SchemaDefProtos.java > 024774a > protocol/src/main/java/org/apache/drill/exec/proto/SchemaUserBitShared.java > 68e86db > protocol/src/main/java/org/apache/drill/exec/proto/UserBitShared.java > 00a4905 > protocol/src/main/java/org/apache/drill/exec/proto/UserProtos.java 330e774 > protocol/src/main/java/org/apache/drill/exec/proto/beans/QueryData.java > PRE-CREATION > protocol/src/main/java/org/apache/drill/exec/proto/beans/QueryResult.java > d8eb92a > protocol/src/main/java/org/apache/drill/exec/proto/beans/RpcType.java > 3f1f9fd > protocol/src/main/protobuf/User.proto 6c41a37 > protocol/src/main/protobuf/UserBitShared.proto 1971e62 > > Diff: https://reviews.apache.org/r/32468/diff/ > > > Testing > ------- > > Unit tests are passing. Can't test functional/customer/tpch100 > > > Thanks, > > abdelhakim deneche > >
