----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34072/#review83636 -----------------------------------------------------------
Some concerns contrib/native/client/src/clientlib/drillClient.cpp <https://reviews.apache.org/r/34072/#comment134644> The use of interval rather than frequency may be more appropriate here. contrib/native/client/src/clientlib/drillClientImpl.cpp <https://reviews.apache.org/r/34072/#comment134651> I would recommend checking if err evaluates to false here otherwise we may end up invoking this logic for other error cases as well. contrib/native/client/src/clientlib/drillClientImpl.cpp <https://reviews.apache.org/r/34072/#comment134652> adding to the above comment using if (!err) we will eliminate this check as well. contrib/native/client/src/clientlib/drillClientImpl.cpp <https://reviews.apache.org/r/34072/#comment134653> As far as I understand this heartbeat protocol is pessimistic in that it gives up on connection in case it fails once. Many heartbeat protocols I have seen usually tries multiple times considering misc problems that may occur. And then there usually is a "grace period" that server attempts last time to hear from client and finally closes the connection. We should take a similar approach here and in Java client. Setting client heartbeat interval < (server hearbeat deadline)/2 such that 2*(heartbeat interval) + grace period = server hearbeat deadline should make it. - Hanifi Gunes On May 13, 2015, 1:38 a.m., Parth Chandra wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/34072/ > ----------------------------------------------------------- > > (Updated May 13, 2015, 1:38 a.m.) > > > Review request for drill, Hanifi Gunes and Norris Lee. > > > Repository: drill-git > > > Description > ------- > > DRILL-2998: Implement heartbeat in C++ client. > > As a result of adding a heartbeat mechanism, the client is always listening > for a response from the hearbeat (i.e always has a request pending). The wait > for results call therefore now returns when the number of query results > pending is zero. Patch also includes a fix for a double delete caused by > copying a pointer to the error object in the broadcastError call. > The patch does not address all cases of detecting when a connection to the > server is gone (this will be addressed in a subsequent fix). > > > Diffs > ----- > > contrib/native/client/example/querySubmitter.cpp > 85e89e0c2429563c411f1e6a8c32f8c8fccf5f03 > contrib/native/client/src/clientlib/drillClient.cpp > 7162f63d16d4107f555c2ed3f66f7fc79e22ee0c > contrib/native/client/src/clientlib/drillClientImpl.hpp > 04d59c763cc9b160ebdba83579863c4856acd701 > contrib/native/client/src/clientlib/drillClientImpl.cpp > eca0e75167620194c848ec38c457c496514653c6 > contrib/native/client/src/include/drill/common.hpp > 2fa09545c6283faa6ff8aa0669ee4cde488088dc > contrib/native/client/src/include/drill/drillClient.hpp > d7bf33c076f37cd244ee8cd604868d9710e450f4 > contrib/native/client/src/protobuf/BitControl.pb.h > 865d3772843d2316e7a5f34fad42621e6360f14d > contrib/native/client/src/protobuf/BitControl.pb.cc > 827f708150df80908e9437a23ec4f915cb5efd7a > contrib/native/client/src/protobuf/GeneralRPC.pb.h > 49f4bf7f7e80939066037183c96d83f140bd7a9e > contrib/native/client/src/protobuf/GeneralRPC.pb.cc > 0ebb3a97c20dc1eabf3ebfc62b649be2fad12443 > contrib/native/client/src/protobuf/User.pb.h > c7deac38a025507bf4da16bb20d0ebce4d84571a > contrib/native/client/src/protobuf/User.pb.cc > 59f215795374e6f22316fe6fa5db588dfe770e9b > contrib/native/client/src/protobuf/UserBitShared.pb.h > e2f5fd0faeddc5968f0cdafd2200f46246210d31 > contrib/native/client/src/protobuf/UserBitShared.pb.cc > b07ecda41b5159458c1d3f7d215afc6835ccf809 > > Diff: https://reviews.apache.org/r/34072/diff/ > > > Testing > ------- > > Tested with broken connections on Mac. Linix and Win64 > > > Thanks, > > Parth Chandra > >
