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/08/07 22:56:12 UTC
Re: Review Request 23941: DRILL-1197: C++ Client. Differentiate
socket/handshake/query timeout for deadline timer.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23941/
-----------------------------------------------------------
(Updated Aug. 7, 2014, 8:56 p.m.)
Review request for drill and Parth Chandra.
Changes
-------
Rebased on DRILL-998 and split the fix of win32 platform to DRILL-1219.
Patch apply order I use:
DRILL-998 -> DRILL-1021 -> DRILL-1137 -> This one.
Bugs: DRILL-1197
https://issues.apache.org/jira/browse/DRILL-1197
Repository: drill-git
Description
-------
- add trace logging for error handling.
- use shutdown operation instead of less portable cancel operation in socket
- return more detailed connection status for validate handshake
- some bug fixes for time out
It depends on DRILL-1021 and DRILL-1137
Diffs (updated)
-----
contrib/native/client/src/clientlib/drillClient.cpp 10480b4
contrib/native/client/src/clientlib/drillClientImpl.hpp 2bf7b5c
contrib/native/client/src/clientlib/drillClientImpl.cpp 6a29d88
contrib/native/client/src/clientlib/errmsgs.hpp 437335c
contrib/native/client/src/clientlib/errmsgs.cpp 966cfc2
contrib/native/client/src/include/drill/common.hpp 2113ce5
contrib/native/client/src/include/drill/drillClient.hpp 6d59afb
Diff: https://reviews.apache.org/r/23941/diff/
Testing
-------
Test case:
1. handshake phase. Run a listening port (say port 1234) and set timeout to 5.
$ nc -l 1234
2. query phase. Run a query like TPCH-12 and set the query timeout with 1 second.
Thanks,
Xiao Meng
Re: Review Request 23941: DRILL-1197: C++ Client. Differentiate
socket/handshake/query timeout for deadline timer.
Posted by Parth Chandra <pc...@maprtech.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23941/#review50067
-----------------------------------------------------------
contrib/native/client/src/clientlib/drillClient.cpp
<https://reviews.apache.org/r/23941/#comment87587>
I would increase this default timeout to 30 secs. 5 seconds is so short it was timing out all the time on my laptop.
contrib/native/client/src/clientlib/drillClientImpl.hpp
<https://reviews.apache.org/r/23941/#comment87610>
The order of includes is : std includes, third party includes, drill client includes.
drill/common.hpp is the only exception and is included first to take care of conflicts bewtween zookeeper headers and windows headers.
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/23941/#comment87589>
In general, the m_pError member is set _after_ you know the error and is not used to determine what the error is or if an error occurred. This change appears necessary if the recvHandshake handler shuts down operation on timeout which we do not want. This change and the change to return status from recvHandshake should really be part of the change to handle recvHandshake timeout for Win32. Please separate this out of this patch.
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/23941/#comment87611>
Please fix indentation. There's a tab in here somewhere.
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/23941/#comment87602>
If the client chooses to connect to a bogus endpoint, then the response will be erroneous the handshake validation will fail.
If the drillbit is hanging, or if the server has not responded, then we would not have reached here at all.
That leaves the case where we get a bogus response, the length of the message is some bogus number and we wait in a loop to read an unreasonable amount of data. If that is the case we should just let a timeout handle the case.
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/23941/#comment87603>
I think we need to cancel the deadline timer at the beginning of this function and start a new one before the while loop.
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/23941/#comment87609>
Please fix indentation. There's a tab in here somewhere.
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/23941/#comment87607>
I don;t think it is OK to start the timer before the thread goes into a wait. The condition wait below is reached if the application has consumed allowed memory and has not released the memory. The application may not have released the memory because it may be waiting for user input or some other reason which could be a long wait. The timer may just go off if that happens.
contrib/native/client/src/clientlib/drillClientImpl.cpp
<https://reviews.apache.org/r/23941/#comment87608>
Instead of this, maybe we can just add a trace in handleQryErr
contrib/native/client/src/include/drill/drillClient.hpp
<https://reviews.apache.org/r/23941/#comment87612>
Please fix indentation. There's a tab in here somewhere.
- Parth Chandra
On Aug. 7, 2014, 8:56 p.m., Xiao Meng wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23941/
> -----------------------------------------------------------
>
> (Updated Aug. 7, 2014, 8:56 p.m.)
>
>
> Review request for drill and Parth Chandra.
>
>
> Bugs: DRILL-1197
> https://issues.apache.org/jira/browse/DRILL-1197
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> - add trace logging for error handling.
> - use shutdown operation instead of less portable cancel operation in socket
> - return more detailed connection status for validate handshake
> - some bug fixes for time out
>
> It depends on DRILL-1021 and DRILL-1137
>
>
> Diffs
> -----
>
> contrib/native/client/src/clientlib/drillClient.cpp 10480b4
> contrib/native/client/src/clientlib/drillClientImpl.hpp 2bf7b5c
> contrib/native/client/src/clientlib/drillClientImpl.cpp 6a29d88
> contrib/native/client/src/clientlib/errmsgs.hpp 437335c
> contrib/native/client/src/clientlib/errmsgs.cpp 966cfc2
> contrib/native/client/src/include/drill/common.hpp 2113ce5
> contrib/native/client/src/include/drill/drillClient.hpp 6d59afb
>
> Diff: https://reviews.apache.org/r/23941/diff/
>
>
> Testing
> -------
>
> Test case:
>
> 1. handshake phase. Run a listening port (say port 1234) and set timeout to 5.
>
> $ nc -l 1234
>
> 2. query phase. Run a query like TPCH-12 and set the query timeout with 1 second.
>
>
> Thanks,
>
> Xiao Meng
>
>
Re: Review Request 23941: DRILL-1197: C++ Client. Differentiate
socket/handshake/query timeout for deadline timer.
Posted by Xiao Meng <xi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23941/
-----------------------------------------------------------
(Updated Nov. 20, 2014, 1:36 a.m.)
Review request for drill and Parth Chandra.
Changes
-------
Rebased and addressed comments.
The client will cancel all async I/O operation when query timed out. Thus, if there is one query timed out, the other queries will be cancelled.
Is it a reasonable choice? Maybe we should use one query timeer for each query but it seems over complicated. Any opinons?
The discussion can be seprated from the patch though.
Bugs: DRILL-1197
https://issues.apache.org/jira/browse/DRILL-1197
Repository: drill-git
Description (updated)
-------
- add trace logging for error handling.
- return more detailed connection status for validate handshake
- some bug fixes for time out
- add options queryTimeout and handshakeTimout to querySubmitter
It depends on DRILL-1679.
Diffs (updated)
-----
contrib/native/client/example/querySubmitter.cpp 7b98bc97b82e142904be97bf5b3ff245c418fdc8
contrib/native/client/src/clientlib/drillClient.cpp 70058ec360f404e473c497cbb86d5e53a98f5bbb
contrib/native/client/src/clientlib/drillClientImpl.hpp 8e2f437f366d6e84cd8b4d6063a7728316421e5d
contrib/native/client/src/clientlib/drillClientImpl.cpp cc70020383b115951488e1552ae1fecb520674fd
contrib/native/client/src/clientlib/errmsgs.hpp 9a69f213cab4ad72c1746ad174b1d711777d255f
contrib/native/client/src/clientlib/errmsgs.cpp 7a7fa6a38bf47ade19e1806b912b03afa29503a2
contrib/native/client/src/include/drill/common.hpp 59537f151dfb2a240222bc3114c06013ee85a2f8
contrib/native/client/src/include/drill/drillClient.hpp 65c07d6a95ca91e25ebbf165f4b54314b9916b22
Diff: https://reviews.apache.org/r/23941/diff/
Testing (updated)
-------
Test case:
1. handshake phase. Run a listening port (say port 1234) and set timeout to 5.
$ nc -l 1234
2. query phase. Run a query like TPCH-12 and set the query timeout with 1 second.
Thanks,
Xiao Meng