You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Kurt Deschler (Code Review)" <ge...@cloudera.org> on 2021/02/25 00:52:48 UTC
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Hello jfs@cloudera.com,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/17116
to review the following change.
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
This patch add a new interface ImpalaServer::GetBackendConfig() that
returns the current TBackendGflags from impalad.
Testing:
Called new interface from external frontend. Verified that
TBackendGflags were populated correctly.
Reviewed-by: John Sherman <jf...@cloudera.com>
Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
---
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M be/src/util/backend-gflag-util.cc
M be/src/util/backend-gflag-util.h
M common/thrift/ImpalaService.thrift
M tests/hs2/test_hs2.py
6 files changed, 64 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/17116/1
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 1
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <jf...@cloudera.com>
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 9:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8255/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 9
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 26 Feb 2021 19:56:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 9:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/rpc/hs2-http-test.cc
File be/src/rpc/hs2-http-test.cc:
http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/rpc/hs2-http-test.cc@53
PS9, Line 53: return_val
nit: we've used '_return' for all of the other functions here
http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:
http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/service/impala-hs2-server.cc@1193
PS9, Line 1193: shared_ptr<SessionState> session;
I don't think the session is actually used for anything here, we're basically just checking that it exists. Any reason to not just leave 'sessionHandle' out of the request entirely and save ourselves some extra work?
http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/util/backend-gflag-util.h
File be/src/util/backend-gflag-util.h:
http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/util/backend-gflag-util.h@27
PS9, Line 27: /// Builds the TBackendGflags object to pass to JNI. This is used to pass the gflag
: /// configs to the Frontend and the Catalog.
It would be cleaner to put this comment directly above the version of the function that it really applies to, i.e. GetThriftBackendGflags
http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/util/backend-gflag-util.h@29
PS9, Line 29: class TBackendGflags;
Not a big deal, but its pretty standard in Impala to put all the forward declaration together at the top, i.e. in this case directly after the "namespace impala {" line above
http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/util/backend-gflag-util.h@31
PS9, Line 31: GetThriftBackendGflags
Might be nice to rename this, eg. to GetThriftBackendGFlagsForJNI, since the difference between GetThriftBackendGflags and PopulateThriftBackendGflags isn't very clear
http://gerrit.cloudera.org:8080/#/c/17116/9/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/17116/9/common/thrift/ImpalaService.thrift@857
PS9, Line 857: // Returns the current TBackendGflags
Maybe mention that this is only supported for the "external fe" server
http://gerrit.cloudera.org:8080/#/c/17116/9/tests/hs2/test_hs2.py
File tests/hs2/test_hs2.py:
http://gerrit.cloudera.org:8080/#/c/17116/9/tests/hs2/test_hs2.py@738
PS9, Line 738: hs2_client
I'm not sure how this test would work, since I would assune that 'hs2_client' here would point at the normal hs2 port, not the "external frontend" port, so shouldn't we hit the "Unsupported operation" error.
Of course, in addition to the case where it works that it tested here, it would be nice to include a test that checks that it gets the error in the cases where it should.
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 9
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 26 Feb 2021 20:41:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 3:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8240/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 3
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 Feb 2021 17:38:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 16:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6952/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 16
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 11 Mar 2021 18:55:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 12:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8269/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 12
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Mar 2021 20:42:09 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Joe McDonnell, jfs@cloudera.com, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17116
to look at the new patch set (#9).
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
This patch add a new interface ImpalaServer::GetBackendConfig() that
returns the current TBackendGflags from impalad.
Testing:
Called new interface from external frontend. Verified that
TBackendGflags were populated correctly.
Reviewed-by: John Sherman <jf...@cloudera.com>
Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
---
M be/src/rpc/hs2-http-test.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M be/src/util/backend-gflag-util.cc
M be/src/util/backend-gflag-util.h
M common/thrift/ImpalaService.thrift
M tests/hs2/test_hs2.py
7 files changed, 73 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/17116/9
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 9
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Hello jfs@cloudera.com, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17116
to look at the new patch set (#2).
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
This patch add a new interface ImpalaServer::GetBackendConfig() that
returns the current TBackendGflags from impalad.
Testing:
Called new interface from external frontend. Verified that
TBackendGflags were populated correctly.
Reviewed-by: John Sherman <jf...@cloudera.com>
Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
---
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M be/src/util/backend-gflag-util.cc
M be/src/util/backend-gflag-util.h
M common/thrift/ImpalaService.thrift
M tests/hs2/test_hs2.py
6 files changed, 65 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/17116/2
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 2
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 2:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/8232/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 2
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 Feb 2021 01:08:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
jfs@cloudera.com has uploaded a new patch set (#6) to the change originally created by Kurt Deschler. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
This patch add a new interface ImpalaServer::GetBackendConfig() that
returns the current TBackendGflags from impalad.
Testing:
Called new interface from external frontend. Verified that
TBackendGflags were populated correctly.
Reviewed-by: John Sherman <jf...@cloudera.com>
Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
---
M be/src/rpc/hs2-http-test.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M be/src/util/backend-gflag-util.cc
M be/src/util/backend-gflag-util.h
M common/thrift/ImpalaService.thrift
M tests/hs2/test_hs2.py
7 files changed, 73 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/17116/6
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 6
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17116/1/tests/hs2/test_hs2.py
File tests/hs2/test_hs2.py:
http://gerrit.cloudera.org:8080/#/c/17116/1/tests/hs2/test_hs2.py@741
PS1, Line 741: )
flake8: E501 line too long (91 > 90 characters)
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 1
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 Feb 2021 00:53:43 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 16: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 16
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Fri, 12 Mar 2021 00:50:04 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Joe McDonnell, John Sherman, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17116
to look at the new patch set (#10).
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
This patch add a new interface ImpalaServer::GetBackendConfig() that
returns the current TBackendGflags from impalad.
Testing:
Called new interface from external frontend. Verified that
TBackendGflags were populated correctly.
Reviewed-by: John Sherman <jf...@cloudera.com>
Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
---
M be/src/catalog/catalog.cc
M be/src/rpc/hs2-http-test.cc
M be/src/service/frontend.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M be/src/util/backend-gflag-util.cc
M be/src/util/backend-gflag-util.h
M be/src/util/logging-support.cc
M common/thrift/ImpalaService.thrift
M tests/hs2/test_hs2.py
10 files changed, 67 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/17116/10
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 10
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 9:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/rpc/hs2-http-test.cc
File be/src/rpc/hs2-http-test.cc:
http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/rpc/hs2-http-test.cc@53
PS9, Line 53: return_val
> nit: we've used '_return' for all of the other functions here
Done
http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:
http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/service/impala-hs2-server.cc@1193
PS9, Line 1193: shared_ptr<SessionState> session;
> I don't think the session is actually used for anything here, we're basical
Done
http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/util/backend-gflag-util.h
File be/src/util/backend-gflag-util.h:
http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/util/backend-gflag-util.h@27
PS9, Line 27: /// Builds the TBackendGflags object to pass to JNI. This is used to pass the gflag
: /// configs to the Frontend and the Catalog.
> It would be cleaner to put this comment directly above the version of the f
Done
http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/util/backend-gflag-util.h@29
PS9, Line 29: class TBackendGflags;
> Not a big deal, but its pretty standard in Impala to put all the forward de
Done
http://gerrit.cloudera.org:8080/#/c/17116/9/be/src/util/backend-gflag-util.h@31
PS9, Line 31: GetThriftBackendGflags
> Might be nice to rename this, eg. to GetThriftBackendGFlagsForJNI, since th
Done
http://gerrit.cloudera.org:8080/#/c/17116/9/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/17116/9/common/thrift/ImpalaService.thrift@857
PS9, Line 857: // Returns the current TBackendGflags
> Maybe mention that this is only supported for the "external fe" server
Done
http://gerrit.cloudera.org:8080/#/c/17116/9/tests/hs2/test_hs2.py
File tests/hs2/test_hs2.py:
http://gerrit.cloudera.org:8080/#/c/17116/9/tests/hs2/test_hs2.py@738
PS9, Line 738: hs2_client
> I'm not sure how this test would work, since I would assune that 'hs2_clien
Done
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 9
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Mar 2021 15:37:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8264/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 10
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Mar 2021 14:20:51 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Joe McDonnell, John Sherman, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17116
to look at the new patch set (#11).
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
This patch add a new interface ImpalaServer::GetBackendConfig() that
returns the current TBackendGflags from impalad.
Testing:
Called new interface from external frontend. Verified that
TBackendGflags were populated correctly.
Reviewed-by: John Sherman <jf...@cloudera.com>
Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
---
M be/src/catalog/catalog.cc
M be/src/rpc/hs2-http-test.cc
M be/src/service/frontend.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M be/src/util/backend-gflag-util.cc
M be/src/util/backend-gflag-util.h
M be/src/util/logging-support.cc
M common/thrift/ImpalaService.thrift
M tests/common/impala_test_suite.py
M tests/conftest.py
M tests/hs2/hs2_test_suite.py
M tests/hs2/test_hs2.py
13 files changed, 83 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/17116/11
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 11
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 11:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8265/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 11
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Mar 2021 16:03:30 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 14:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17116/14/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:
http://gerrit.cloudera.org:8080/#/c/17116/14/be/src/service/impala-hs2-server.cc@1190
PS14, Line 1190: HS2_RETURN_IF_ERROR(return_val, THandleIdentifierToTUniqueId(
> I think you may have misunderstood my previous comment here, sorry if I was
This RPC uses common Client and Session logic on the external frontend to connect to Impalad. Originally it was not clear if we might need authentication for external frontend connections. The external port was added later but then rebased before this change to make it more clear how security would work and avoid having an interim security hole. I'd prefer to leave this as-is for now.
http://gerrit.cloudera.org:8080/#/c/17116/14/be/src/util/backend-gflag-util.cc
File be/src/util/backend-gflag-util.cc:
http://gerrit.cloudera.org:8080/#/c/17116/14/be/src/util/backend-gflag-util.cc@258
PS14, Line 258: string saml2_private_key_password;
> As is, this patch will return things from GetBackendConfig that are both se
The intention with external frontends is to share Impala frontend code and functionality. As such, we have not limited access to backend metadata which may be needed in unforeseen places, especially as more functionality is added. As it stands, this mechanism simply mirrors what the Impala FE is getting, excepted pulled rather than pushed. The log case as easier to make as restricting access there is not going to cause functional problems.
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 14
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Wed, 03 Mar 2021 02:57:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 11:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/17116/11/tests/hs2/hs2_test_suite.py
File tests/hs2/hs2_test_suite.py:
http://gerrit.cloudera.org:8080/#/c/17116/11/tests/hs2/hs2_test_suite.py@123
PS11, Line 123:
flake8: E251 unexpected spaces around keyword / parameter equals
http://gerrit.cloudera.org:8080/#/c/17116/11/tests/hs2/hs2_test_suite.py@123
PS11, Line 123:
flake8: E251 unexpected spaces around keyword / parameter equals
http://gerrit.cloudera.org:8080/#/c/17116/11/tests/hs2/test_hs2.py
File tests/hs2/test_hs2.py:
http://gerrit.cloudera.org:8080/#/c/17116/11/tests/hs2/test_hs2.py@740
PS11, Line 740: "
flake8: E501 line too long (114 > 90 characters)
http://gerrit.cloudera.org:8080/#/c/17116/11/tests/hs2/test_hs2.py@742
PS11, Line 742: _
flake8: E501 line too long (96 > 90 characters)
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 11
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Mar 2021 15:37:28 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Hello jfs@cloudera.com, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17116
to look at the new patch set (#3).
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
This patch add a new interface ImpalaServer::GetBackendConfig() that
returns the current TBackendGflags from impalad.
Testing:
Called new interface from external frontend. Verified that
TBackendGflags were populated correctly.
Reviewed-by: John Sherman <jf...@cloudera.com>
Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
---
M be/src/rpc/hs2-http-test.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M be/src/util/backend-gflag-util.cc
M be/src/util/backend-gflag-util.h
M common/thrift/ImpalaService.thrift
M tests/hs2/test_hs2.py
7 files changed, 67 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/17116/3
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 3
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 1:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/8231/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 1
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 Feb 2021 01:04:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 12:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/17116/12/tests/hs2/test_hs2.py
File tests/hs2/test_hs2.py:
http://gerrit.cloudera.org:8080/#/c/17116/12/tests/hs2/test_hs2.py@743
PS12, Line 743: _
flake8: E501 line too long (96 > 90 characters)
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 12
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 01 Mar 2021 20:09:29 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 13:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8283/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 13
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Mar 2021 15:52:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
This patch add a new interface ImpalaServer::GetBackendConfig() that
returns the current TBackendGflags from impalad.
Testing:
Called new interface from external frontend. Verified that
TBackendGflags were populated correctly.
Reviewed-by: John Sherman <jf...@cloudera.com>
Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Reviewed-on: http://gerrit.cloudera.org:8080/17116
Reviewed-by: Thomas Tauber-Marshall <tm...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/catalog/catalog.cc
M be/src/rpc/hs2-http-test.cc
M be/src/service/frontend.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M be/src/util/backend-gflag-util.cc
M be/src/util/backend-gflag-util.h
M be/src/util/logging-support.cc
M common/thrift/ImpalaService.thrift
M tests/hs2/test_hs2.py
10 files changed, 64 insertions(+), 6 deletions(-)
Approvals:
Thomas Tauber-Marshall: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 17
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 16: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 16
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Mon, 08 Mar 2021 17:20:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 14:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/17116/14/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:
http://gerrit.cloudera.org:8080/#/c/17116/14/be/src/service/impala-hs2-server.cc@1190
PS14, Line 1190: HS2_RETURN_IF_ERROR(return_val, THandleIdentifierToTUniqueId(
I think you may have misunderstood my previous comment here, sorry if I was unclear.
Is there any reason for this rpc to take 'sessionHandle' at all, i.e. TGetBackendConfigReq.sessionHandle? I don't think it will ever be the case that the value of backend flags will be session-dependent, so removing TGetBackendConfigReq.sessionHandle would increase flexibility - eg. presumably this will just be getting called once at startup, when there isn't an actual user session anyways, so removing 'sessionHandle' would mean the external FE doesn't have to make a fake session just to call this.
I guess one consideration is authentication - maybe we only want users that can create sessions to be able to call this - but I don't think that comes into play here since this will only be accessible over the external FE interface where authentication is always turned off.
I may be missing something about how this will eventually evolve, and its not that big of a deal since like I said this will presumably only be called once at startup so perf doesn't really matter, so I'm fine if you prefer to leave it as is.
http://gerrit.cloudera.org:8080/#/c/17116/14/be/src/util/backend-gflag-util.cc
File be/src/util/backend-gflag-util.cc:
http://gerrit.cloudera.org:8080/#/c/17116/14/be/src/util/backend-gflag-util.cc@258
PS14, Line 258: string saml2_private_key_password;
As is, this patch will return things from GetBackendConfig that are both sensitive and (I assume) unnecessary for the external FE, such as passwords.
Since we're restricting this to use by the external FE, which we implicitly trust, maybe that's fine, but it would be nice if we had some way to remove this kind of stuff from what gets returned, eg. by depending on the TAG_FLAG(sensitive)/CheckFlagAndRedact construct like we do to prevent these sorts of things from being printed in the logs or output on the debug webui, eg. see: https://github.com/apache/impala/blob/master/be/src/util/default-path-handlers.cc#L105
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 14
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Tue, 02 Mar 2021 19:28:12 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Joe McDonnell, John Sherman, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17116
to look at the new patch set (#12).
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
This patch add a new interface ImpalaServer::GetBackendConfig() that
returns the current TBackendGflags from impalad.
Testing:
Called new interface from external frontend. Verified that
TBackendGflags were populated correctly.
Reviewed-by: John Sherman <jf...@cloudera.com>
Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
---
M be/src/catalog/catalog.cc
M be/src/rpc/hs2-http-test.cc
M be/src/service/frontend.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M be/src/util/backend-gflag-util.cc
M be/src/util/backend-gflag-util.h
M be/src/util/logging-support.cc
M common/thrift/ImpalaService.thrift
M tests/common/impala_test_suite.py
M tests/conftest.py
M tests/hs2/hs2_test_suite.py
M tests/hs2/test_hs2.py
13 files changed, 84 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/17116/12
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 12
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Kurt Deschler (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Joe McDonnell, John Sherman, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17116
to look at the new patch set (#13).
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
This patch add a new interface ImpalaServer::GetBackendConfig() that
returns the current TBackendGflags from impalad.
Testing:
Called new interface from external frontend. Verified that
TBackendGflags were populated correctly.
Reviewed-by: John Sherman <jf...@cloudera.com>
Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
---
M be/src/catalog/catalog.cc
M be/src/rpc/hs2-http-test.cc
M be/src/service/frontend.cc
M be/src/service/impala-hs2-server.cc
M be/src/service/impala-server.h
M be/src/util/backend-gflag-util.cc
M be/src/util/backend-gflag-util.h
M be/src/util/logging-support.cc
M common/thrift/ImpalaService.thrift
M tests/hs2/test_hs2.py
10 files changed, 64 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/16/17116/13
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 13
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: John Sherman <jf...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/17116 )
Change subject: IMPALA-10546: Add ImpalaServer interface to retrieve BackendConfig from impalad
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/8247/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/17116
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14a3cee29f1fc91f4431b7ea89053bb3fbfa5e69
Gerrit-Change-Number: 17116
Gerrit-PatchSet: 6
Gerrit-Owner: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <jf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <jo...@cloudera.com>
Gerrit-Reviewer: Kurt Deschler <kd...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 25 Feb 2021 23:50:38 +0000
Gerrit-HasComments: No