> On March 31, 2015, 9:36 p.m., Chris Westin wrote: > > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/UserResultsListener.java, > > line 27 > > <https://reviews.apache.org/r/32468/diff/2/?file=907350#file907350line27> > > > > I'm a little surprised this doesn't include the final query status in > > some form (the new QueryResult?).
I kept the behavior of the client listener the same. The original code didn't even wait for the final query state except for failed queries. I will update this as it may be useful if a client wants to handle cancelled queries separately (I suppose it's the only case where a client needs to check the query state for a completed query). > On March 31, 2015, 9:36 p.m., Chris Westin wrote: > > exec/java-exec/src/test/java/org/apache/drill/SingleRowListener.java, line > > 62 > > <https://reviews.apache.org/r/32468/diff/2/?file=907356#file907356line62> > > > > I'll bet that cleanup() can throw, so to avoid this causing hangs, > > perhaps it should be wrapped with a try-finally here. How to handle the exception, just report it in the logs ? > On March 31, 2015, 9:36 p.m., Chris Westin wrote: > > protocol/src/main/protobuf/UserBitShared.proto, line 41 > > <https://reviews.apache.org/r/32468/diff/2/?file=907382#file907382line41> > > > > In order to deal with the new suppressed exceptions, won't we need to > > add a > > "repeated ExceptionWrapper suppressed = 5" to this? But how is this change relevant to this patch ? > On March 31, 2015, 9:36 p.m., Chris Westin wrote: > > protocol/src/main/protobuf/UserBitShared.proto, line 110 > > <https://reviews.apache.org/r/32468/diff/2/?file=907382#file907382line110> > > > > 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. all removed fields were never set nor used. If submission_time is needed, then we may need to fix the code to set this value when sending a QueryResult - abdelhakim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32468/#review78086 ----------------------------------------------------------- On March 27, 2015, 1:20 a.m., abdelhakim deneche wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32468/ > ----------------------------------------------------------- > > (Updated March 27, 2015, 1:20 a.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 > >
