You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Xiao Meng <xi...@gmail.com> on 2014/09/06 02:50:09 UTC
Review Request 25409: DRILL-1305: C++ Client. Consume QueryState
message from the Drillbit.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25409/
-----------------------------------------------------------
Review request for drill and Parth Chandra.
Repository: drill-git
Description
-------
DRILL-1305: C++ Client. Consume QueryState message from the Drillbit.
Diffs
-----
contrib/native/client/src/clientlib/drillClientImpl.hpp e40b2147cc7057b5c0a2b6b6f32afafed236a928
contrib/native/client/src/clientlib/drillClientImpl.cpp 54dcdd0f1dcc1e9608b996c47fd6f61e8f0f21c2
contrib/native/client/src/clientlib/recordBatch.cpp 17073bd6284cb5e4825cca6a6ddf5c34ee850af3
contrib/native/client/src/include/drill/common.hpp 2113ce5e14d07ee91b6a9a34efb68ae33f617707
Diff: https://reviews.apache.org/r/25409/diff/
Testing
-------
Thanks,
Xiao Meng
Re: Review Request 25409: DRILL-1305: C++ Client. Consume QueryState
message from the Drillbit.
Posted by Parth Chandra <pc...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25409/#review54341
-----------------------------------------------------------
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/25409/#comment94465>
The understanding was that the callback would always have a non-NULL record batch pointer. In this case however, the record batch is NULL.
The querySubmitter program crashes when this code path is hit.
Also, the default query results listener (and the synchronous version of the library which uses this) break because of the NULL record batch.
- Parth Chandra
On Sept. 18, 2014, 10:07 p.m., Xiao Meng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25409/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2014, 10:07 p.m.)
>
>
> Review request for drill and Parth Chandra.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-1305: C++ Client. Consume QueryState message from the Drillbit.
>
>
> Diffs
> -----
>
> contrib/native/client/src/clientlib/drillClientImpl.hpp 5ac158f
> contrib/native/client/src/clientlib/drillClientImpl.cpp f1a165d
> contrib/native/client/src/clientlib/errmsgs.hpp 437335c
> contrib/native/client/src/clientlib/errmsgs.cpp 966cfc2
> contrib/native/client/src/include/drill/common.hpp 151d698
>
> Diff: https://reviews.apache.org/r/25409/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Xiao Meng
>
>
Re: Review Request 25409: DRILL-1305: C++ Client. Consume QueryState
message from the Drillbit.
Posted by Xiao Meng <xi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25409/#review55103
-----------------------------------------------------------
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/25409/#comment95464>
The query_state checking here is redudant now since it is been checked in the switch cases.
- Xiao Meng
On Sept. 29, 2014, 7:46 p.m., Xiao Meng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25409/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2014, 7:46 p.m.)
>
>
> Review request for drill and Parth Chandra.
>
>
> Bugs: DRILL-1305
> https://issues.apache.org/jira/browse/DRILL-1305
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-1305: C++ Client. Consume QueryState message from the Drillbit.
>
>
> Diffs
> -----
>
> contrib/native/client/example/querySubmitter.cpp 17bec3c
> contrib/native/client/src/clientlib/drillClientImpl.hpp 5ac158f
> contrib/native/client/src/clientlib/drillClientImpl.cpp f1a165d
> contrib/native/client/src/clientlib/errmsgs.hpp 437335c
> contrib/native/client/src/clientlib/errmsgs.cpp 966cfc2
> contrib/native/client/src/include/drill/common.hpp 151d698
>
> Diff: https://reviews.apache.org/r/25409/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Xiao Meng
>
>
Re: Review Request 25409: DRILL-1305: C++ Client. Consume QueryState
message from the Drillbit.
Posted by Parth Chandra <pc...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25409/#review57752
-----------------------------------------------------------
Ship it!
Ship It!
- Parth Chandra
On Oct. 15, 2014, 10:15 p.m., Xiao Meng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25409/
> -----------------------------------------------------------
>
> (Updated Oct. 15, 2014, 10:15 p.m.)
>
>
> Review request for drill, Alexander zarei, Norris Lee, and Parth Chandra.
>
>
> Bugs: DRILL-1305
> https://issues.apache.org/jira/browse/DRILL-1305
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-1305: C++ Client. Consume QueryState message from the Drillbit.
>
>
> Diffs
> -----
>
> contrib/native/client/example/querySubmitter.cpp f886e819f43d21e07bb3df9956817783d69b98bd
> contrib/native/client/src/clientlib/drillClientImpl.hpp d690aad0e7ba5d006404036cb3c392d0b62a0ca9
> contrib/native/client/src/clientlib/drillClientImpl.cpp 77795ed08367df58ae2ac43ef80ae16d5066ba91
> contrib/native/client/src/clientlib/errmsgs.hpp 437335c560833875a3b50d0ee934f849e3a0af9e
> contrib/native/client/src/clientlib/errmsgs.cpp 966cfc27be180bcaa3f46f437389b60fdaece7f5
> contrib/native/client/src/include/drill/common.hpp 151d698b53fe0fa5f153b585448cb5536f2967be
>
> Diff: https://reviews.apache.org/r/25409/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Xiao Meng
>
>
Re: Review Request 25409: DRILL-1305: C++ Client. Consume QueryState
message from the Drillbit.
Posted by Xiao Meng <xi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25409/
-----------------------------------------------------------
(Updated Oct. 15, 2014, 10:15 p.m.)
Review request for drill, Alexander zarei, Norris Lee, and Parth Chandra.
Changes
-------
Rebased on master and tried a new approach based on comments. This patch depends on DRILL-1511 to fix compling issue on Mac.
Here. we always ensure that in queryResultListener, the recordBatch is not empty if DrillClientError object is NULL.
```cpp
QueryResultsListener(void* ctx, Drill::RecordBatch* b, Drill::DrillClientError* err)
```
When the DrillClientError object is not NULL, it shows the status of the query. In the case of receiving an QUERY_COMPLETED message, the status is QRY_COMPLETED.
(Use the name DrillClientError may not appricaite here since it is not an "error" anymore).
To implement this, this patch implements `handleTerminatedQryState` similar to `handleQryError`.
Any opions on this approach?
Thanks,
Xiao
Bugs: DRILL-1305
https://issues.apache.org/jira/browse/DRILL-1305
Repository: drill-git
Description
-------
DRILL-1305: C++ Client. Consume QueryState message from the Drillbit.
Diffs (updated)
-----
contrib/native/client/example/querySubmitter.cpp f886e819f43d21e07bb3df9956817783d69b98bd
contrib/native/client/src/clientlib/drillClientImpl.hpp d690aad0e7ba5d006404036cb3c392d0b62a0ca9
contrib/native/client/src/clientlib/drillClientImpl.cpp 77795ed08367df58ae2ac43ef80ae16d5066ba91
contrib/native/client/src/clientlib/errmsgs.hpp 437335c560833875a3b50d0ee934f849e3a0af9e
contrib/native/client/src/clientlib/errmsgs.cpp 966cfc27be180bcaa3f46f437389b60fdaece7f5
contrib/native/client/src/include/drill/common.hpp 151d698b53fe0fa5f153b585448cb5536f2967be
Diff: https://reviews.apache.org/r/25409/diff/
Testing
-------
Thanks,
Xiao Meng
Re: Review Request 25409: DRILL-1305: C++ Client. Consume QueryState
message from the Drillbit.
Posted by Xiao Meng <xi...@gmail.com>.
> On Sept. 29, 2014, 11 p.m., Parth Chandra wrote:
> > contrib/native/client/src/clientlib/drillClientImpl.cpp, line 907
> > <https://reviews.apache.org/r/25409/diff/3/?file=708334#file708334line907>
> >
> > This line crashes when the record batch passed in is NULL. I'm not sure what the result will be if we set the m_bHasData flag to true and signal the waiting thread but have not actually added a new record batch to the queue. I'm almost sure that the drill client sync API will croak.
> > Basically, passing in a NULL record batch to the query results listener seems to violate the implicit contract that the listener will be called only when there are results.
I see. So we'd better handle those NULL record batch in DrillClient::processQueryResult (as I commented) instead of calling query results listener?
- Xiao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25409/#review54909
-----------------------------------------------------------
On Sept. 29, 2014, 7:46 p.m., Xiao Meng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25409/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2014, 7:46 p.m.)
>
>
> Review request for drill and Parth Chandra.
>
>
> Bugs: DRILL-1305
> https://issues.apache.org/jira/browse/DRILL-1305
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-1305: C++ Client. Consume QueryState message from the Drillbit.
>
>
> Diffs
> -----
>
> contrib/native/client/example/querySubmitter.cpp 17bec3c
> contrib/native/client/src/clientlib/drillClientImpl.hpp 5ac158f
> contrib/native/client/src/clientlib/drillClientImpl.cpp f1a165d
> contrib/native/client/src/clientlib/errmsgs.hpp 437335c
> contrib/native/client/src/clientlib/errmsgs.cpp 966cfc2
> contrib/native/client/src/include/drill/common.hpp 151d698
>
> Diff: https://reviews.apache.org/r/25409/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Xiao Meng
>
>
Re: Review Request 25409: DRILL-1305: C++ Client. Consume QueryState
message from the Drillbit.
Posted by Xiao Meng <xi...@gmail.com>.
- Xiao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25409/#review54909
-----------------------------------------------------------
On Sept. 29, 2014, 7:46 p.m., Xiao Meng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25409/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2014, 7:46 p.m.)
>
>
> Review request for drill and Parth Chandra.
>
>
> Bugs: DRILL-1305
> https://issues.apache.org/jira/browse/DRILL-1305
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-1305: C++ Client. Consume QueryState message from the Drillbit.
>
>
> Diffs
> -----
>
> contrib/native/client/example/querySubmitter.cpp 17bec3c
> contrib/native/client/src/clientlib/drillClientImpl.hpp 5ac158f
> contrib/native/client/src/clientlib/drillClientImpl.cpp f1a165d
> contrib/native/client/src/clientlib/errmsgs.hpp 437335c
> contrib/native/client/src/clientlib/errmsgs.cpp 966cfc2
> contrib/native/client/src/include/drill/common.hpp 151d698
>
> Diff: https://reviews.apache.org/r/25409/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Xiao Meng
>
>
Re: Review Request 25409: DRILL-1305: C++ Client. Consume QueryState
message from the Drillbit.
Posted by Parth Chandra <pc...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25409/#review54909
-----------------------------------------------------------
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/25409/#comment95242>
This line crashes when the record batch passed in is NULL. I'm not sure what the result will be if we set the m_bHasData flag to true and signal the waiting thread but have not actually added a new record batch to the queue. I'm almost sure that the drill client sync API will croak.
Basically, passing in a NULL record batch to the query results listener seems to violate the implicit contract that the listener will be called only when there are results.
- Parth Chandra
On Sept. 29, 2014, 7:46 p.m., Xiao Meng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25409/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2014, 7:46 p.m.)
>
>
> Review request for drill and Parth Chandra.
>
>
> Bugs: DRILL-1305
> https://issues.apache.org/jira/browse/DRILL-1305
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-1305: C++ Client. Consume QueryState message from the Drillbit.
>
>
> Diffs
> -----
>
> contrib/native/client/example/querySubmitter.cpp 17bec3c
> contrib/native/client/src/clientlib/drillClientImpl.hpp 5ac158f
> contrib/native/client/src/clientlib/drillClientImpl.cpp f1a165d
> contrib/native/client/src/clientlib/errmsgs.hpp 437335c
> contrib/native/client/src/clientlib/errmsgs.cpp 966cfc2
> contrib/native/client/src/include/drill/common.hpp 151d698
>
> Diff: https://reviews.apache.org/r/25409/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Xiao Meng
>
>
Re: Review Request 25409: DRILL-1305: C++ Client. Consume QueryState
message from the Drillbit.
Posted by Xiao Meng <xi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25409/#review54914
-----------------------------------------------------------
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/25409/#comment95250>
We should not call the query result listener here if we want to keep the contract that recordBatch is always not null for QueryResultListener.
- Xiao Meng
On Sept. 29, 2014, 7:46 p.m., Xiao Meng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25409/
> -----------------------------------------------------------
>
> (Updated Sept. 29, 2014, 7:46 p.m.)
>
>
> Review request for drill and Parth Chandra.
>
>
> Bugs: DRILL-1305
> https://issues.apache.org/jira/browse/DRILL-1305
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-1305: C++ Client. Consume QueryState message from the Drillbit.
>
>
> Diffs
> -----
>
> contrib/native/client/example/querySubmitter.cpp 17bec3c
> contrib/native/client/src/clientlib/drillClientImpl.hpp 5ac158f
> contrib/native/client/src/clientlib/drillClientImpl.cpp f1a165d
> contrib/native/client/src/clientlib/errmsgs.hpp 437335c
> contrib/native/client/src/clientlib/errmsgs.cpp 966cfc2
> contrib/native/client/src/include/drill/common.hpp 151d698
>
> Diff: https://reviews.apache.org/r/25409/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Xiao Meng
>
>
Re: Review Request 25409: DRILL-1305: C++ Client. Consume QueryState
message from the Drillbit.
Posted by Xiao Meng <xi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25409/
-----------------------------------------------------------
(Updated Sept. 29, 2014, 7:46 p.m.)
Review request for drill and Parth Chandra.
Bugs: DRILL-1305
https://issues.apache.org/jira/browse/DRILL-1305
Repository: drill-git
Description
-------
DRILL-1305: C++ Client. Consume QueryState message from the Drillbit.
Diffs
-----
contrib/native/client/example/querySubmitter.cpp 17bec3c
contrib/native/client/src/clientlib/drillClientImpl.hpp 5ac158f
contrib/native/client/src/clientlib/drillClientImpl.cpp f1a165d
contrib/native/client/src/clientlib/errmsgs.hpp 437335c
contrib/native/client/src/clientlib/errmsgs.cpp 966cfc2
contrib/native/client/src/include/drill/common.hpp 151d698
Diff: https://reviews.apache.org/r/25409/diff/
Testing
-------
Thanks,
Xiao Meng
Re: Review Request 25409: DRILL-1305: C++ Client. Consume QueryState
message from the Drillbit.
Posted by Xiao Meng <xi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25409/
-----------------------------------------------------------
(Updated Sept. 29, 2014, 7:45 p.m.)
Review request for drill and Parth Chandra.
Changes
-------
update querySumbmitter and tacke review request.
Repository: drill-git
Description
-------
DRILL-1305: C++ Client. Consume QueryState message from the Drillbit.
Diffs (updated)
-----
contrib/native/client/example/querySubmitter.cpp 17bec3c
contrib/native/client/src/clientlib/drillClientImpl.hpp 5ac158f
contrib/native/client/src/clientlib/drillClientImpl.cpp f1a165d
contrib/native/client/src/clientlib/errmsgs.hpp 437335c
contrib/native/client/src/clientlib/errmsgs.cpp 966cfc2
contrib/native/client/src/include/drill/common.hpp 151d698
Diff: https://reviews.apache.org/r/25409/diff/
Testing
-------
Thanks,
Xiao Meng
Re: Review Request 25409: DRILL-1305: C++ Client. Consume QueryState
message from the Drillbit.
Posted by Parth Chandra <pc...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25409/#review54184
-----------------------------------------------------------
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/25409/#comment94184>
Is the io_service.reset needed here? What happens if the io_service is reset and there were requests pending? If m_pListenerThread is null then there are no more requests pending, but in this case there might be a query with results pending.
- Parth Chandra
On Sept. 18, 2014, 10:07 p.m., Xiao Meng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25409/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2014, 10:07 p.m.)
>
>
> Review request for drill and Parth Chandra.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-1305: C++ Client. Consume QueryState message from the Drillbit.
>
>
> Diffs
> -----
>
> contrib/native/client/src/clientlib/drillClientImpl.hpp 5ac158f
> contrib/native/client/src/clientlib/drillClientImpl.cpp f1a165d
> contrib/native/client/src/clientlib/errmsgs.hpp 437335c
> contrib/native/client/src/clientlib/errmsgs.cpp 966cfc2
> contrib/native/client/src/include/drill/common.hpp 151d698
>
> Diff: https://reviews.apache.org/r/25409/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Xiao Meng
>
>
Re: Review Request 25409: DRILL-1305: C++ Client. Consume QueryState
message from the Drillbit.
Posted by Xiao Meng <xi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25409/
-----------------------------------------------------------
(Updated Sept. 18, 2014, 10:07 p.m.)
Review request for drill and Parth Chandra.
Changes
-------
rebased on master, revised on comments and exclude some unrelated changes from the patch.
Repository: drill-git
Description
-------
DRILL-1305: C++ Client. Consume QueryState message from the Drillbit.
Diffs (updated)
-----
contrib/native/client/src/clientlib/drillClientImpl.hpp 5ac158f
contrib/native/client/src/clientlib/drillClientImpl.cpp f1a165d
contrib/native/client/src/clientlib/errmsgs.hpp 437335c
contrib/native/client/src/clientlib/errmsgs.cpp 966cfc2
contrib/native/client/src/include/drill/common.hpp 151d698
Diff: https://reviews.apache.org/r/25409/diff/
Testing
-------
Thanks,
Xiao Meng
Re: Review Request 25409: DRILL-1305: C++ Client. Consume QueryState
message from the Drillbit.
Posted by Xiao Meng <xi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25409/#review52936
-----------------------------------------------------------
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/25409/#comment92171>
the first one should be drillClientImpl.hpp. Since this cpp file is mainly for the implemenation of drillClientImpl.hpp.
Refer C++ Code guideline here
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Names_and_Order_of_Includes
- Xiao Meng
On Sept. 6, 2014, 12:50 a.m., Xiao Meng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25409/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2014, 12:50 a.m.)
>
>
> Review request for drill and Parth Chandra.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-1305: C++ Client. Consume QueryState message from the Drillbit.
>
>
> Diffs
> -----
>
> contrib/native/client/src/clientlib/drillClientImpl.hpp e40b2147cc7057b5c0a2b6b6f32afafed236a928
> contrib/native/client/src/clientlib/drillClientImpl.cpp 54dcdd0f1dcc1e9608b996c47fd6f61e8f0f21c2
> contrib/native/client/src/clientlib/recordBatch.cpp 17073bd6284cb5e4825cca6a6ddf5c34ee850af3
> contrib/native/client/src/include/drill/common.hpp 2113ce5e14d07ee91b6a9a34efb68ae33f617707
>
> Diff: https://reviews.apache.org/r/25409/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Xiao Meng
>
>
Re: Review Request 25409: DRILL-1305: C++ Client. Consume QueryState
message from the Drillbit.
Posted by Xiao Meng <xi...@gmail.com>.
> On Sept. 10, 2014, 7:19 p.m., Parth Chandra wrote:
> > contrib/native/client/src/clientlib/drillClientImpl.cpp, line 19
> > <https://reviews.apache.org/r/25409/diff/1/?file=681791#file681791line19>
> >
> > Order of header files should be -
> >
> > system files (System headers, then STL, then C lib headers)
> >
> > third party headers (boost etc)
> >
> > drill public headers
> >
> > drill private headers
> >
> > drill/common.hpp is the only one that breaks the rule and can be included as the first file to prevent conflicts with Zookeeper headers.
will move this changes to another patch.
- Xiao
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25409/#review52912
-----------------------------------------------------------
On Sept. 18, 2014, 10:07 p.m., Xiao Meng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25409/
> -----------------------------------------------------------
>
> (Updated Sept. 18, 2014, 10:07 p.m.)
>
>
> Review request for drill and Parth Chandra.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-1305: C++ Client. Consume QueryState message from the Drillbit.
>
>
> Diffs
> -----
>
> contrib/native/client/src/clientlib/drillClientImpl.hpp 5ac158f
> contrib/native/client/src/clientlib/drillClientImpl.cpp f1a165d
> contrib/native/client/src/clientlib/errmsgs.hpp 437335c
> contrib/native/client/src/clientlib/errmsgs.cpp 966cfc2
> contrib/native/client/src/include/drill/common.hpp 151d698
>
> Diff: https://reviews.apache.org/r/25409/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Xiao Meng
>
>
Re: Review Request 25409: DRILL-1305: C++ Client. Consume QueryState
message from the Drillbit.
Posted by Parth Chandra <pc...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25409/#review52912
-----------------------------------------------------------
contrib/native/client/src/clientlib/drillClientImpl.hpp
<https://reviews.apache.org/r/25409/#comment92122>
Not needed. Zookeeper header files already have this
contrib/native/client/src/clientlib/drillClientImpl.hpp
<https://reviews.apache.org/r/25409/#comment92123>
Please remove extra whitespace
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/25409/#comment92124>
Order of header files should be -
system files (System headers, then STL, then C lib headers)
third party headers (boost etc)
drill public headers
drill private headers
drill/common.hpp is the only one that breaks the rule and can be included as the first file to prevent conflicts with Zookeeper headers.
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/25409/#comment92125>
See order of header files comment above
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/25409/#comment92126>
Please fix indentation. Looks like a tab here.
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/25409/#comment92146>
This change appears to be unrelated. More importantly, consider the implications of this change.
Boost is picky about the order of the send and receive. Multiple parallel sends or receives will result in grief. The client library must ensure is does not do things like :
send
recv
recv
send
or
send
send
recv
recv
Calling m_io_service post will probably queue up two parallel sends and the results will be unpredictable. So this is an unsafe change.
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/25409/#comment92131>
m_pendingRequests should be decrements ONLY when the last message has been received. If the query state is PENDING or RUNNINg, the counter should not be decremented.
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/25409/#comment92133>
Consider raising an error with a user friendly error message as opposed to an assert.
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/25409/#comment92138>
The << operator is not needed here since the compiler will concatenate two adjacent string literals. Does no harm though.
contrib/native/client/src/clientlib/recordBatch.cpp
<https://reviews.apache.org/r/25409/#comment92140>
Is this include needed here?
contrib/native/client/src/include/drill/common.hpp
<https://reviews.apache.org/r/25409/#comment92142>
The indentation seems to be off. Is there a tab here (lines 79-82)
- Parth Chandra
On Sept. 6, 2014, 12:50 a.m., Xiao Meng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25409/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2014, 12:50 a.m.)
>
>
> Review request for drill and Parth Chandra.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> DRILL-1305: C++ Client. Consume QueryState message from the Drillbit.
>
>
> Diffs
> -----
>
> contrib/native/client/src/clientlib/drillClientImpl.hpp e40b2147cc7057b5c0a2b6b6f32afafed236a928
> contrib/native/client/src/clientlib/drillClientImpl.cpp 54dcdd0f1dcc1e9608b996c47fd6f61e8f0f21c2
> contrib/native/client/src/clientlib/recordBatch.cpp 17073bd6284cb5e4825cca6a6ddf5c34ee850af3
> contrib/native/client/src/include/drill/common.hpp 2113ce5e14d07ee91b6a9a34efb68ae33f617707
>
> Diff: https://reviews.apache.org/r/25409/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Xiao Meng
>
>