You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by superbstreak <gi...@git.apache.org> on 2017/03/07 02:18:24 UTC
[GitHub] drill pull request #772: DRILL-5316: Check drillbitsVector count from zoo_ge...
GitHub user superbstreak opened a pull request:
https://github.com/apache/drill/pull/772
DRILL-5316: Check drillbitsVector count from zoo_get_children before \u2026
\u2026we attempt to access the vector element
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/superbstreak/drill DRILL-5316
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/drill/pull/772.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 #772
----
commit c9ca5a35aa684992d58d93e50a7a9ff7ce16ead8
Author: Rob Wu <ro...@gmail.com>
Date: 2017-03-07T02:17:25Z
DRILL-5316: Check drillbitsVector count from zoo_get_children before we attempt to access the vector element
----
---
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 #772: DRILL-5316: Check drillbits size before we attempt ...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/772#discussion_r105286121
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -2143,6 +2146,9 @@ connectionStatus_t PooledDrillClientImpl::connect(const char* connStr, DrillUser
Utils::shuffle(drillbits);
// The original shuffled order is maintained if we shuffle first and then add any missing elements
Utils::add(m_drillbits, drillbits);
+ if (m_drillbits.empty()){
+ return handleConnError(CONN_FAILURE, getMessage(ERR_CONN_ZKNODBIT));
--- End diff --
Since we are not removing the offline nodes from m_drillbits then I think we should return connection error before shuffle. Let's say on first client connection we get all the active node from zookeeper and store it in m_drillbits. Then all the nodes went dead or offline. In the next connection request, zookeeper will return zero drillbits but since m_drillbits is not empty we will still try to connect and fail later.
Instead I think zero drillbits returned from zookeeper is a good indication that we won't be able to connect to any other node already present inside m_drillbits and should fail there itself ?
---
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 issue #772: DRILL-5316: Check drillbits size before we attempt to acce...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on the issue:
https://github.com/apache/drill/pull/772
Thanks @superbstreak for the change.
LGTM. +1 from my side.
---
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 #772: DRILL-5316: Check drillbitsVector count from zoo_ge...
Posted by sohami <gi...@git.apache.org>.
Github user sohami commented on a diff in the pull request:
https://github.com/apache/drill/pull/772#discussion_r104606833
--- Diff: contrib/native/client/src/clientlib/zookeeperClient.cpp ---
@@ -138,6 +138,11 @@ int ZookeeperClient::getAllDrillbits(const std::string& connectStr, std::vector<
DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "\t Unshuffled Drillbit id: " << drillbits[i] << std::endl;)
}
}
+ else{
--- End diff --
Agreed. Should be handled in caller (i.e. DrillClient). If the returned vector size is zero then we should check that in DrillClient and close client connection with error as `ERR_CONN_ZKNODBIT`. Something like below:
`return handleConnError(CONN_INVALID_INPUT, getMessage(ERR_CONN_ZKNODBIT, pathToDrill.c_str()));`
---
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 #772: DRILL-5316: Check drillbits size before we attempt ...
Posted by superbstreak <gi...@git.apache.org>.
Github user superbstreak commented on a diff in the pull request:
https://github.com/apache/drill/pull/772#discussion_r105046142
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -2143,6 +2146,9 @@ connectionStatus_t PooledDrillClientImpl::connect(const char* connStr, DrillUser
Utils::shuffle(drillbits);
// The original shuffled order is maintained if we shuffle first and then add any missing elements
Utils::add(m_drillbits, drillbits);
+ if (m_drillbits.empty()){
+ return handleConnError(CONN_FAILURE, getMessage(ERR_CONN_ZKNODBIT));
--- End diff --
Thanks, @laurentgo! A new ticket has been filed for the suggestion: [DRILL-5335](https://issues.apache.org/jira/browse/DRILL-5335)
---
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 issue #772: DRILL-5316: Check drillbits size before we attempt to acce...
Posted by superbstreak <gi...@git.apache.org>.
Github user superbstreak commented on the issue:
https://github.com/apache/drill/pull/772
@sohami please review.
---
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 #772: DRILL-5316: Check drillbitsVector count from zoo_ge...
Posted by superbstreak <gi...@git.apache.org>.
Github user superbstreak commented on a diff in the pull request:
https://github.com/apache/drill/pull/772#discussion_r104627768
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -86,6 +86,9 @@ connectionStatus_t DrillClientImpl::connect(const char* connStr, DrillUserProper
std::vector<std::string> drillbits;
int err = zook.getAllDrillbits(hostPortStr, drillbits);
if(!err){
+ if (drillbits.empty()){
+ return handleConnError(CONN_INVALID_INPUT, getMessage(ERR_CONN_ZKNODBIT));
--- End diff --
double check CONN_INVALID_INPUT usage.
---
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 issue #772: DRILL-5316: Check drillbits size before we attempt to acce...
Posted by amansinha100 <gi...@git.apache.org>.
Github user amansinha100 commented on the issue:
https://github.com/apache/drill/pull/772
+1
---
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 #772: DRILL-5316: Check drillbitsVector count from zoo_ge...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/772#discussion_r104594602
--- Diff: contrib/native/client/src/clientlib/zookeeperClient.cpp ---
@@ -138,6 +138,11 @@ int ZookeeperClient::getAllDrillbits(const std::string& connectStr, std::vector<
DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "\t Unshuffled Drillbit id: " << drillbits[i] << std::endl;)
}
}
+ else{
--- End diff --
I'm not sure the check should be in the zookeeper client, but instead in the drill client: the zookeeper client is just returning the content of the zookeeper tree, and there's no error from the zookeeper client point of view (so no need to disconnect/reconnect).
---
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 #772: DRILL-5316: Check drillbitsVector count from zoo_ge...
Posted by superbstreak <gi...@git.apache.org>.
Github user superbstreak commented on a diff in the pull request:
https://github.com/apache/drill/pull/772#discussion_r104607726
--- Diff: contrib/native/client/src/clientlib/zookeeperClient.cpp ---
@@ -138,6 +138,11 @@ int ZookeeperClient::getAllDrillbits(const std::string& connectStr, std::vector<
DRILL_MT_LOG(DRILL_LOG(LOG_TRACE) << "\t Unshuffled Drillbit id: " << drillbits[i] << std::endl;)
}
}
+ else{
--- End diff --
Thanks both. Yea make sense, I'll make the change.
---
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 #772: DRILL-5316: Check drillbits size before we attempt ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/drill/pull/772
---
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 #772: DRILL-5316: Check drillbits size before we attempt ...
Posted by laurentgo <gi...@git.apache.org>.
Github user laurentgo commented on a diff in the pull request:
https://github.com/apache/drill/pull/772#discussion_r105034016
--- Diff: contrib/native/client/src/clientlib/drillClientImpl.cpp ---
@@ -2143,6 +2146,9 @@ connectionStatus_t PooledDrillClientImpl::connect(const char* connStr, DrillUser
Utils::shuffle(drillbits);
// The original shuffled order is maintained if we shuffle first and then add any missing elements
Utils::add(m_drillbits, drillbits);
+ if (m_drillbits.empty()){
+ return handleConnError(CONN_FAILURE, getMessage(ERR_CONN_ZKNODBIT));
--- End diff --
can be done before the shuffle (it should not change the number of elements)
---
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.
---