You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by sudheeshkatkam <gi...@git.apache.org> on 2017/01/24 21:18:46 UTC
[GitHub] drill pull request #726: DRILL-5218: Support disabling hearbeats from C++ cl...
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 <su...@apache.org>
Date: 2017-01-24T21:10:31Z
DRILL-5218: Support disabling hearbeats from C++ client
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #726: DRILL-5218: Support disabling hearbeats from C++ cl...
Posted by laurentgo <gi...@git.apache.org>.
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #726: DRILL-5218: Support disabling hearbeats from C++ cl...
Posted by laurentgo <gi...@git.apache.org>.
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...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #726: DRILL-5218: Support disabling hearbeats from C++ cl...
Posted by sudheeshkatkam <gi...@git.apache.org>.
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`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] drill pull request #726: DRILL-5218: Support disabling hearbeats from C++ cl...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/726
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---