drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "abdelhakim deneche" <adene...@gmail.com>
Subject Re: Review Request 32468: DRILL-2498: Separate QueryResult into two messages QueryResult and QueryData
Date Wed, 01 Apr 2015 18:17:13 GMT


> On March 31, 2015, 9:36 p.m., Chris Westin wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/user/QueryResultHandler.java,
line 353
> > <https://reviews.apache.org/r/32468/diff/2/?file=907348#file907348line353>
> >
> >     How come output needs to be protected, but throttle doesn't?

My understanding is that BufferingListener keeps track of the latest throttle received in
dataArrived (does it even change ?) so it can pass it to another listener when transferTo()
is called.


> 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.
> 
> abdelhakim deneche wrote:
>     How to handle the exception, just report it in the logs ?

missed "finally" keyword in your comment =P


> On March 31, 2015, 9:36 p.m., Chris Westin wrote:
> > exec/jdbc/src/main/java/org/apache/drill/jdbc/DrillResultSet.java, line 154
> > <https://reviews.apache.org/r/32468/diff/2/?file=907366#file907366line154>
> >
> >     Why did you leave it here then?

Didn't want to introduce any side-effects, and I was expecting to do another patch anyway.


- 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message