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