[jira] [Commented] (DRILL-5218) Support Disabling Heartbeats in C++ Client
[ https://issues.apache.org/jira/browse/DRILL-5218?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15844193#comment-15844193 ] Parth Chandra commented on DRILL-5218: -- Until we fix DRILL-5217, it seems this might be the only way to get past cases where the client app cannot pull in data fast enough or does not have enough buffer memory to handle a large result set. As long as this is optional I guess it's ok to commit this change. +1. > Support Disabling Heartbeats in C++ Client > -- > > Key: DRILL-5218 > URL: https://issues.apache.org/jira/browse/DRILL-5218 > Project: Apache Drill > Issue Type: Bug > Components: Client - C++ >Reporter: Sudheesh Katkam >Assignee: Sudheesh Katkam > > Heartbeats between bits allow for detecting health of remotes, but heartbeats > between client and bit may not be necessary? So allow to (at least) disable > heartbeats between C++ client and bit. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (DRILL-5218) Support Disabling Heartbeats in C++ Client
[
https://issues.apache.org/jira/browse/DRILL-5218?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15838736#comment-15838736
]
ASF GitHub Bot commented on DRILL-5218:
---
Github user sudheeshkatkam commented on a diff in the pull request:
https://github.com/apache/drill/pull/726#discussion_r97896146
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -179,9 +179,11 @@ connectionStatus_t DrillClientImpl::sendHeartbeat(){
}
void DrillClientImpl::resetHeartbeatTimer(){
-m_heartbeatTimer.cancel();
-DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Reset Heartbeat timer." <<
std::endl;)
-startHeartbeatTimer();
+if (DrillClientConfig::getHeartbeatFrequency() > 0) {
+m_heartbeatTimer.cancel();
--- End diff --
You are right, cancelling is not required because
`timer.expires_from_now(...)` cancels the timer as well. So I will remove this
method and move the check to `startHeartbeatTimer`.
> Support Disabling Heartbeats in C++ Client
> --
>
> Key: DRILL-5218
> URL: https://issues.apache.org/jira/browse/DRILL-5218
> Project: Apache Drill
> Issue Type: Bug
> Components: Client - C++
>Reporter: Sudheesh Katkam
>Assignee: Sudheesh Katkam
>
> Heartbeats between bits allow for detecting health of remotes, but heartbeats
> between client and bit may not be necessary? So allow to (at least) disable
> heartbeats between C++ client and bit.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (DRILL-5218) Support Disabling Heartbeats in C++ Client
[
https://issues.apache.org/jira/browse/DRILL-5218?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15838589#comment-15838589
]
Sudheesh Katkam commented on DRILL-5218:
Are heartbeats beneficial? I guess so (thanks to [~parthc] for pointing me to
[this
article|http://blog.stephencleary.com/2009/05/detection-of-half-open-dropped.html]).
But if heartbeats are disabled, the client does not know if the connection is
alive, but the next message to the server would fail, and the client could
reconnect, and if that fails, the client knows something is really wrong. If
the query execution is taking too long, the server could send the status of a
query once a while (which is different from heartbeats during idle time).
Is the heartbeat mechanism implemented correctly? I don't think so, I read [an
article| http://250bpm.com/blog:22] that explains what is required to implement
heartbeats correctly, and Drill does not meet the checklist.
{quote}
...
Thus, when you are evaluating a solution that implements heartbeating on top of
TCP here is a checklist to help you to find out whether it actually works:
1. If there are heartbeats but there's no flow control, the solution won't work.
2. There must be a way to split larger messages into smaller units that will
fit into the preallocated buffer. If there's no such mechanism, the solution
won't work.
3. The credit in the flow-control mechanism must be expressed in terms of
bytes, not messages. If it uses number of messages to control the flow, the
solution won't work.
4. If the protocol allows to issue more credit than the space available in the
receive buffer, the solution won't work.
...
{quote}
Until all issues (including DRILL-5217) are resolved, I suggest providing an
option per connection to disable heartbeats. Fair?
> Support Disabling Heartbeats in C++ Client
> --
>
> Key: DRILL-5218
> URL: https://issues.apache.org/jira/browse/DRILL-5218
> Project: Apache Drill
> Issue Type: Bug
> Components: Client - C++
>Reporter: Sudheesh Katkam
>Assignee: Sudheesh Katkam
>
> Heartbeats between bits allow for detecting health of remotes, but heartbeats
> between client and bit may not be necessary? So allow to (at least) disable
> heartbeats between C++ client and bit.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (DRILL-5218) Support Disabling Heartbeats in C++ Client
[
https://issues.apache.org/jira/browse/DRILL-5218?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15836750#comment-15836750
]
Laurent Goujon commented on DRILL-5218:
---
https://issues.apache.org/jira/browse/DRILL-5218?focusedCommentId=15836652&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-15836652
{quote}
Doesn't sound like a good idea. A heartbeat is the only reliable way of knowing
that the connection to a drillbit is still alive (and not just taking a very
long time to return the results of a query execution). The client app might be
a server application maintaining a connection pool in which case knowing the
health of the connection is an important requirement.
{quote}
I was wondering about that too. [~sudheeshkatkam], do you have an exemple where
it would be beneficial to disable the heartbeat?
> Support Disabling Heartbeats in C++ Client
> --
>
> Key: DRILL-5218
> URL: https://issues.apache.org/jira/browse/DRILL-5218
> Project: Apache Drill
> Issue Type: Bug
> Components: Client - C++
>Reporter: Sudheesh Katkam
>Assignee: Sudheesh Katkam
>
> Heartbeats between bits allow for detecting health of remotes, but heartbeats
> between client and bit may not be necessary? So allow to (at least) disable
> heartbeats between C++ client and bit.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (DRILL-5218) Support Disabling Heartbeats in C++ Client
[ https://issues.apache.org/jira/browse/DRILL-5218?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15836746#comment-15836746 ] ASF GitHub Bot commented on DRILL-5218: --- Github user laurentgo commented on a diff in the pull request: https://github.com/apache/drill/pull/726#discussion_r97663988 --- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp --- @@ -1400,22 +1404,6 @@ void DrillClientImpl::handleRead(ByteBuf_t _buf, break; case exec::user::HANDSHAKE: --- End diff -- maybe you can also remove the case statement > Support Disabling Heartbeats in C++ Client > -- > > Key: DRILL-5218 > URL: https://issues.apache.org/jira/browse/DRILL-5218 > Project: Apache Drill > Issue Type: Bug > Components: Client - C++ >Reporter: Sudheesh Katkam >Assignee: Sudheesh Katkam > > Heartbeats between bits allow for detecting health of remotes, but heartbeats > between client and bit may not be necessary? So allow to (at least) disable > heartbeats between C++ client and bit. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (DRILL-5218) Support Disabling Heartbeats in C++ Client
[
https://issues.apache.org/jira/browse/DRILL-5218?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15836747#comment-15836747
]
ASF GitHub Bot commented on DRILL-5218:
---
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/726#discussion_r97665284
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -179,9 +179,11 @@ connectionStatus_t DrillClientImpl::sendHeartbeat(){
}
void DrillClientImpl::resetHeartbeatTimer(){
-m_heartbeatTimer.cancel();
-DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "Reset Heartbeat timer." <<
std::endl;)
-startHeartbeatTimer();
+if (DrillClientConfig::getHeartbeatFrequency() > 0) {
+m_heartbeatTimer.cancel();
--- End diff --
I was wondering if one needs to cancel the timer, as startHearbeatTimer
sets it again to expire. Maybe all this logic can be done in one place (like
startHeartbeatTimer?)
I also noticed another place where m_heartbeatTime.cancel() is called, in
broadcastError. I guess this is fine (probably not an error of calling cancel()
if not set, but haven't checked asio doc on it), but maybe this should be
cleaned up/guarded too...
> Support Disabling Heartbeats in C++ Client
> --
>
> Key: DRILL-5218
> URL: https://issues.apache.org/jira/browse/DRILL-5218
> Project: Apache Drill
> Issue Type: Bug
> Components: Client - C++
>Reporter: Sudheesh Katkam
>Assignee: Sudheesh Katkam
>
> Heartbeats between bits allow for detecting health of remotes, but heartbeats
> between client and bit may not be necessary? So allow to (at least) disable
> heartbeats between C++ client and bit.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
[jira] [Commented] (DRILL-5218) Support Disabling Heartbeats in C++ Client
[ https://issues.apache.org/jira/browse/DRILL-5218?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15836652#comment-15836652 ] Parth Chandra commented on DRILL-5218: -- Doesn't sound like a good idea. A heartbeat is the only reliable way of knowing that the connection to a drillbit is still alive (and not just taking a very long time to return the results of a query execution). The client app might be a server application maintaining a connection pool in which case knowing the health of the connection is an important requirement. > Support Disabling Heartbeats in C++ Client > -- > > Key: DRILL-5218 > URL: https://issues.apache.org/jira/browse/DRILL-5218 > Project: Apache Drill > Issue Type: Bug > Components: Client - C++ >Reporter: Sudheesh Katkam >Assignee: Sudheesh Katkam > > Heartbeats between bits allow for detecting health of remotes, but heartbeats > between client and bit may not be necessary? So allow to (at least) disable > heartbeats between C++ client and bit. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (DRILL-5218) Support Disabling Heartbeats in C++ Client
[ https://issues.apache.org/jira/browse/DRILL-5218?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15836647#comment-15836647 ] ASF GitHub Bot commented on DRILL-5218: --- GitHub user sudheeshkatkam opened a pull request: https://github.com/apache/drill/pull/726 DRILL-5218: Support disabling hearbeats from C++ client + remove invalid code (server should not request "handshake" type), in fact, client should fail in that case You can merge this pull request into a Git repository by running: $ git pull https://github.com/sudheeshkatkam/drill DRILL-5218 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/drill/pull/726.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #726 commit 6bc07bbba246ae376b6accc4e38f076bb15b83aa Author: Sudheesh Katkam Date: 2017-01-24T21:10:31Z DRILL-5218: Support disabling hearbeats from C++ client > Support Disabling Heartbeats in C++ Client > -- > > Key: DRILL-5218 > URL: https://issues.apache.org/jira/browse/DRILL-5218 > Project: Apache Drill > Issue Type: Bug > Components: Client - C++ >Reporter: Sudheesh Katkam >Assignee: Sudheesh Katkam > > Heartbeats between bits allow for detecting health of remotes, but heartbeats > between client and bit may not be necessary? So allow to (at least) disable > heartbeats between C++ client and bit. -- This message was sent by Atlassian JIRA (v6.3.4#6332)
