You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Andrew Sherman (Code Review)" <ge...@cloudera.org> on 2018/09/17 23:32:17 UTC
[Impala-ASF-CR] IMPALA-589: Add a sql function to return the impalad coordinator hostname for diagnostic purposes.
Andrew Sherman has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11459
Change subject: IMPALA-589: Add a sql function to return the impalad coordinator hostname for diagnostic purposes.
......................................................................
IMPALA-589: Add a sql function to return the impalad coordinator hostname for diagnostic purposes.
In every execution of an Impala query, one of the impalad daemons acts as
the coordinator node. In some cases, such as when using a proxy, a user
cannot predict which host will act as the coordinator. To aid in diagnosis,
we provide a sql function which returns the name of the host on which the
coordinator is running.
EXTERNAL DESCRIPTION:
Add a builtin function called coordinator(), which returns the name of the
host which is running the impalad that is acting as the coordinator for the
current query.
IMPLEMENTATION:
All functions are passed a FunctionContext object which is the interface to the
rest of the system for a UDF. From the FunctionContext we get the TQueryCtx
(query context) which contains a TNetworkAddress for the coordinator. The
hostname of the coordinator is copied from the TNetworkAddress.
Can the TNetworkAddress for the coordinator be unitialized? In the thrift
source the coord_address is marked optional, but my reading of the code
says this is always set in a real impalad. To future-proof the code a null
StringVal is returned if coord_address is unset.
TESTING:
- Added a basic unit test for the new function.
- Added a unit test which simulates the case when coord_address is unset.
- Hand tested in a development deployment.
- Ran regression tests and got a clean run.
LIMITATIONS:
This change only adds the coordinator() function.
It does not add the debug_url() function that was mentioned in the comments on
the original Jira.
Change-Id: I94d6e2664ba659b48df53c5c06f67b502c533e47
---
M be/src/exprs/expr-test.cc
M be/src/exprs/utility-functions-ir.cc
M be/src/exprs/utility-functions.h
M common/function-registry/impala_functions.py
M docs/topics/impala_misc_functions.xml
5 files changed, 61 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/59/11459/1
--
To view, visit http://gerrit.cloudera.org:8080/11459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I94d6e2664ba659b48df53c5c06f67b502c533e47
Gerrit-Change-Number: 11459
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
[Impala-ASF-CR] IMPALA-589: Add sql function returning the impalad coordinator hostname.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11459 )
Change subject: IMPALA-589: Add sql function returning the impalad coordinator hostname.
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/11459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94d6e2664ba659b48df53c5c06f67b502c533e47
Gerrit-Change-Number: 11459
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Sep 2018 20:26:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-589: Add sql function returning the impalad coordinator hostname.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/11459 )
Change subject: IMPALA-589: Add sql function returning the impalad coordinator hostname.
......................................................................
Patch Set 2:
(1 comment)
Thanks for making me look at the *.test files
http://gerrit.cloudera.org:8080/#/c/11459/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11459/2//COMMIT_MSG@7
PS2, Line 7: IMPALA-589: Add sql function returning the impalad coordinator hostname.
> I appreciate your thoroughness, but we generally make our commit messages a
Done
--
To view, visit http://gerrit.cloudera.org:8080/11459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94d6e2664ba659b48df53c5c06f67b502c533e47
Gerrit-Change-Number: 11459
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Sep 2018 16:09:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-589: Add sql function returning the impalad coordinator hostname.
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11459 )
Change subject: IMPALA-589: Add sql function returning the impalad coordinator hostname.
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/11459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94d6e2664ba659b48df53c5c06f67b502c533e47
Gerrit-Change-Number: 11459
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Sep 2018 16:44:25 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-589: Add sql function returning the impalad coordinator hostname.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/11459 )
Change subject: IMPALA-589: Add sql function returning the impalad coordinator hostname.
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3203/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/11459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94d6e2664ba659b48df53c5c06f67b502c533e47
Gerrit-Change-Number: 11459
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Mon, 24 Sep 2018 16:44:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-589: Add sql function returning the impalad coordinator hostname.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/11459 )
Change subject: IMPALA-589: Add sql function returning the impalad coordinator hostname.
......................................................................
IMPALA-589: Add sql function returning the impalad coordinator hostname.
In every execution of an Impala query, one of the impalad daemons acts
as the coordinator node. In some cases, such as when using a proxy, a
user cannot predict which host will act as the coordinator. To aid in
diagnosis, we provide a sql function which returns the name of the host
on which the coordinator is running.
EXTERNAL DESCRIPTION:
Add a builtin function called coordinator(), which returns the name of
the host which is running the impalad that is acting as the coordinator
for the current query.
IMPLEMENTATION:
All functions are passed a FunctionContext object which is the interface
to the rest of the system for a UDF. From the FunctionContext we get
the TQueryCtx (query context) which contains a TNetworkAddress for
the coordinator. The hostname of the coordinator is copied from the
TNetworkAddress.
Can the TNetworkAddress for the coordinator be unitialized? In the
thrift source the coord_address is marked optional, but my reading of
the code says this is always set in a real impalad. To future-proof the
code a null StringVal is returned if coord_address is unset.
TESTING:
- Added a basic unit test for the new function.
- Added a unit test which simulates the case when coord_address is
unset.
- Hand tested in a development deployment.
- Ran regression tests and got a clean run.
LIMITATIONS:
This change only adds the coordinator() function.
It does not add the debug_url() function that was mentioned in the
comments on the original Jira.
Change-Id: I94d6e2664ba659b48df53c5c06f67b502c533e47
---
M be/src/exprs/expr-test.cc
M be/src/exprs/utility-functions-ir.cc
M be/src/exprs/utility-functions.h
M common/function-registry/impala_functions.py
M docs/topics/impala_misc_functions.xml
5 files changed, 60 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/59/11459/2
--
To view, visit http://gerrit.cloudera.org:8080/11459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94d6e2664ba659b48df53c5c06f67b502c533e47
Gerrit-Change-Number: 11459
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
[Impala-ASF-CR] IMPALA-589: Add sql function returning the impalad coordinator hostname.
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11459 )
Change subject: IMPALA-589: Add sql function returning the impalad coordinator hostname.
......................................................................
IMPALA-589: Add sql function returning the impalad coordinator hostname.
In every execution of an Impala query, one of the impalad daemons acts
as the coordinator node. In some cases, such as when using a proxy, a
user cannot predict which host will act as the coordinator. To aid in
diagnosis, we provide a sql function which returns the name of the host
on which the coordinator is running.
EXTERNAL DESCRIPTION:
Add a builtin function called coordinator(), which returns the name of
the host which is running the impalad that is acting as the coordinator
for the current query.
TESTING:
- Added a basic unit test for the new function.
- Added a unit test which simulates the case when coord_address is
unset.
- Added a query that uses coordinator() to exprs.test
- Hand tested in a development deployment.
- Ran regression tests and got a clean run.
Change-Id: I94d6e2664ba659b48df53c5c06f67b502c533e47
Reviewed-on: http://gerrit.cloudera.org:8080/11459
Reviewed-by: Thomas Marshall <th...@cmu.edu>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/exprs/expr-test.cc
M be/src/exprs/utility-functions-ir.cc
M be/src/exprs/utility-functions.h
M common/function-registry/impala_functions.py
M docs/topics/impala_misc_functions.xml
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
6 files changed, 66 insertions(+), 0 deletions(-)
Approvals:
Thomas Marshall: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/11459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I94d6e2664ba659b48df53c5c06f67b502c533e47
Gerrit-Change-Number: 11459
Gerrit-PatchSet: 4
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
[Impala-ASF-CR] IMPALA-589: Add sql function returning the impalad coordinator hostname.
Posted by "Thomas Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/11459 )
Change subject: IMPALA-589: Add sql function returning the impalad coordinator hostname.
......................................................................
Patch Set 2:
(1 comment)
Can you also add an e2e test in testdata/workloads/functional-query/queries/QueryTest/exprs.text?
http://gerrit.cloudera.org:8080/#/c/11459/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/11459/2//COMMIT_MSG@7
PS2, Line 7: IMPALA-589: Add sql function returning the impalad coordinator hostname.
I appreciate your thoroughness, but we generally make our commit messages a little more concise (esp. for such a small, straight-forward change).
In particular, you can probably eliminate the "Implementation" section here, and you can definitely eliminate the "Limitations" section.
--
To view, visit http://gerrit.cloudera.org:8080/11459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94d6e2664ba659b48df53c5c06f67b502c533e47
Gerrit-Change-Number: 11459
Gerrit-PatchSet: 2
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Sep 2018 22:53:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-589: Add sql function returning the impalad coordinator hostname.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/11459 )
Change subject: IMPALA-589: Add sql function returning the impalad coordinator hostname.
......................................................................
IMPALA-589: Add sql function returning the impalad coordinator hostname.
In every execution of an Impala query, one of the impalad daemons acts
as the coordinator node. In some cases, such as when using a proxy, a
user cannot predict which host will act as the coordinator. To aid in
diagnosis, we provide a sql function which returns the name of the host
on which the coordinator is running.
EXTERNAL DESCRIPTION:
Add a builtin function called coordinator(), which returns the name of
the host which is running the impalad that is acting as the coordinator
for the current query.
TESTING:
- Added a basic unit test for the new function.
- Added a unit test which simulates the case when coord_address is
unset.
- Added a query that uses coordinator() to exprs.test
- Hand tested in a development deployment.
- Ran regression tests and got a clean run.
Change-Id: I94d6e2664ba659b48df53c5c06f67b502c533e47
---
M be/src/exprs/expr-test.cc
M be/src/exprs/utility-functions-ir.cc
M be/src/exprs/utility-functions.h
M common/function-registry/impala_functions.py
M docs/topics/impala_misc_functions.xml
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
6 files changed, 66 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/59/11459/3
--
To view, visit http://gerrit.cloudera.org:8080/11459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I94d6e2664ba659b48df53c5c06f67b502c533e47
Gerrit-Change-Number: 11459
Gerrit-PatchSet: 3
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
[Impala-ASF-CR] IMPALA-589: Add sql function returning the impalad coordinator hostname.
Posted by "Andrew Sherman (Code Review)" <ge...@cloudera.org>.
Andrew Sherman has posted comments on this change. ( http://gerrit.cloudera.org:8080/11459 )
Change subject: IMPALA-589: Add sql function returning the impalad coordinator hostname.
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11459/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
http://gerrit.cloudera.org:8080/#/c/11459/1/be/src/exprs/expr-test.cc@4961
PS1, Line 4961: ObjectPool pool;
> This does not seem to be used.
Thanks
--
To view, visit http://gerrit.cloudera.org:8080/11459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94d6e2664ba659b48df53c5c06f67b502c533e47
Gerrit-Change-Number: 11459
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Sep 2018 00:38:03 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-589: Add a sql function to return the impalad coordinator hostname for diagnostic purposes.
Posted by "Zoram Thanga (Code Review)" <ge...@cloudera.org>.
Zoram Thanga has posted comments on this change. ( http://gerrit.cloudera.org:8080/11459 )
Change subject: IMPALA-589: Add a sql function to return the impalad coordinator hostname for diagnostic purposes.
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Change looks good to me. It would be great to have the function take an optional query id argument so we can use it on a running query as well. But I realize that that's a lot of work, and the same info is discoverable from the CM.
http://gerrit.cloudera.org:8080/#/c/11459/1/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:
http://gerrit.cloudera.org:8080/#/c/11459/1/be/src/exprs/expr-test.cc@4961
PS1, Line 4961: ObjectPool pool;
This does not seem to be used.
--
To view, visit http://gerrit.cloudera.org:8080/11459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I94d6e2664ba659b48df53c5c06f67b502c533e47
Gerrit-Change-Number: 11459
Gerrit-PatchSet: 1
Gerrit-Owner: Andrew Sherman <as...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Zoram Thanga <zo...@cloudera.com>
Gerrit-Comment-Date: Wed, 19 Sep 2018 23:06:04 +0000
Gerrit-HasComments: Yes