[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Hello Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13672 to look at the new patch set (#2). Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server .. IMPALA-8584: Add cookie support to the HTTP HS2 server This patch modifies the HTTP HS2 server to accept cookies for authentication in order to avoid having to authenticate every request through LDAP. It adds a flag, --max_cookie_lifetime_s, that determines how long generated cookies are valid for. Setting the flag to 0 disables cookie support. It also adds the metrics: impala.thrift-server.hiveserver2-http-frontend.total-cookie-auth-success impala.thrift-server.hiveserver2-http-frontend.total-cookie-auth-failure Testing: - Added tests to the FE LDAP tests. Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4 --- M be/src/rpc/CMakeLists.txt M be/src/rpc/TAcceptQueueServer.cpp M be/src/rpc/auth-provider.h M be/src/rpc/authentication.cc A be/src/rpc/cookie-mgr.cc A be/src/rpc/cookie-mgr.h M be/src/rpc/thrift-server.cc M be/src/runtime/exec-env.cc M be/src/runtime/exec-env.h M be/src/transport/THttpServer.cpp M be/src/transport/THttpServer.h M common/thrift/metrics.json M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java M fe/src/test/java/org/apache/impala/service/JdbcTestBase.java 15 files changed, 486 insertions(+), 63 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/13672/2 -- To view, visit http://gerrit.cloudera.org:8080/13672 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4 Gerrit-Change-Number: 13672 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] IMPALA-8584: Add cookie support to the HTTP HS2 server
Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13672 Change subject: IMPALA-8584: Add cookie support to the HTTP HS2 server .. IMPALA-8584: Add cookie support to the HTTP HS2 server This patch modifies the HTTP HS2 server to accept cookies for authentication in order to avoid having to authenticate every request through LDAP. Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4 --- M be/src/rpc/CMakeLists.txt M be/src/rpc/TAcceptQueueServer.cpp M be/src/rpc/auth-provider.h M be/src/rpc/authentication.cc A be/src/rpc/cookie-mgr.cc A be/src/rpc/cookie-mgr.h M be/src/rpc/thrift-server.cc M be/src/runtime/exec-env.cc M be/src/runtime/exec-env.h M be/src/transport/THttpServer.cpp M be/src/transport/THttpServer.h M common/thrift/metrics.json M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java M fe/src/test/java/org/apache/impala/service/JdbcTestBase.java 15 files changed, 486 insertions(+), 63 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/13672/1 -- To view, visit http://gerrit.cloudera.org:8080/13672 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I647c06f94ef91aa3b6413e91576c4ec506ed57f4 Gerrit-Change-Number: 13672 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall
[Impala-ASF-CR] IMPALA-7467: Ported ExecQueryFInstances over to krpc
Thomas Marshall has posted comments on this change. (
http://gerrit.cloudera.org:8080/13583 )
Change subject: IMPALA-7467: Ported ExecQueryFInstances over to krpc
..
Patch Set 2:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/runtime/coordinator-backend-state.cc
File be/src/runtime/coordinator-backend-state.cc:
http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/runtime/coordinator-backend-state.cc@176
PS1, Line 176: LOG(ERROR)
> LOG(ERROR) ?
Done
http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/runtime/coordinator-backend-state.cc@209
PS1, Line 209: serialized_buf
> May help to add a remark about the ownership of this buffer.
Done
http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/runtime/coordinator-backend-state.cc@214
PS1, Line 214: SetExecError(serialize_st
> Why is this necessary ?
Done
http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/runtime/coordinator-backend-state.cc@218
PS1, Line 218: Status::Expected("Ser
> Same as above.
Done
http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/runtime/coordinator-backend-state.cc@233
PS1, Line 233: quest.set_sidecar_idx(sidecar_idx);
> If we fail to add the sidecar, it doesn't seem to make sense to continue wi
Done
http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/service/control-service.h
File be/src/service/control-service.h:
http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/service/control-service.h@112
PS1, Line 112: /// call within 'rpc_context'.
> Is this necessary ? What's wrong with getting it from the singleton ExecEnv
Done
http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/service/control-service.cc
File be/src/service/control-service.cc:
http://gerrit.cloudera.org:8080/#/c/13583/1/be/src/service/control-service.cc@73
PS1, Line 73: }
:
> Move to initializer list.
Done
http://gerrit.cloudera.org:8080/#/c/13583/1/common/thrift/ImpalaInternalService.thrift
File common/thrift/ImpalaInternalService.thrift:
http://gerrit.cloudera.org:8080/#/c/13583/1/common/thrift/ImpalaInternalService.thrift@579
PS1, Line 579: // TODO: convert this fully to protobuf.
> Do we plan to clean it up eventually to use protobuf only ? How about leavi
Yeah, but that's a potentially very large change (eg. requires rewriting all of
the toThrift()s for Exprs/PlanNodes in the FE to toProtobuf()s), so probably
makes sense to do it in pieces, if possible
--
To view, visit http://gerrit.cloudera.org:8080/13583
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id3f1c6099109bd8e5361188005a7d0e892147570
Gerrit-Change-Number: 13583
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Marshall
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Tue, 18 Jun 2019 03:21:53 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7467: Ported ExecQueryFInstances over to krpc
Hello Michael Ho, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13583 to look at the new patch set (#2). Change subject: IMPALA-7467: Ported ExecQueryFInstances over to krpc .. IMPALA-7467: Ported ExecQueryFInstances over to krpc This patch ports the ExecQueryFInstances rpc to use KRPC. The parameters for this call contain a huge number of Thrift structs (eg. everything related to TPlanNode and TExpr), so to avoid converting all of these to protobuf and the resulting effect that would have on the FE and catalog, this patch stores most of the parameters in a sidecar (in particular the TQueryCtx, TPlanFragmentCtx's, and TPlanFragmentInstanceCtx's). Testing: - Passed a full exhaustive run on the minicluster. Set up a ten node cluster with tpch 500: - Ran perf tests: 3 iterations per tpch query, 4 concurrent streams, no perf change. - Ran the stress test for 1000 queries, passed. Change-Id: Id3f1c6099109bd8e5361188005a7d0e892147570 --- M be/src/runtime/backend-client.h M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/query-exec-mgr.cc M be/src/runtime/query-exec-mgr.h M be/src/runtime/query-state.cc M be/src/runtime/query-state.h M be/src/runtime/test-env.cc M be/src/service/control-service.cc M be/src/service/control-service.h M be/src/service/impala-internal-service.cc M be/src/service/impala-internal-service.h M common/protobuf/control_service.proto M common/thrift/ImpalaInternalService.thrift M tests/custom_cluster/test_rpc_timeout.py 15 files changed, 242 insertions(+), 171 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/13583/2 -- To view, visit http://gerrit.cloudera.org:8080/13583 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Id3f1c6099109bd8e5361188005a7d0e892147570 Gerrit-Change-Number: 13583 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho
[Impala-ASF-CR] IMPALA-7290: part 2: Add HS2 support to Impala shell
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12884 ) Change subject: IMPALA-7290: part 2: Add HS2 support to Impala shell .. Patch Set 22: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/12884/22/shell/impala_shell.py File shell/impala_shell.py: http://gerrit.cloudera.org:8080/#/c/12884/22/shell/impala_shell.py@1178 PS22, Line 1178: # self.close_connection() ? -- To view, visit http://gerrit.cloudera.org:8080/12884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6d5cc83d545aacc659523f29b1d6feed672e2a12 Gerrit-Change-Number: 12884 Gerrit-PatchSet: 22 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 14 Jun 2019 18:09:29 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8583: Add metrics for Basic auth
Hello Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13640 to look at the new patch set (#3). Change subject: IMPALA-8583: Add metrics for Basic auth .. IMPALA-8583: Add metrics for Basic auth The patch adds two metrics: impala.thrift-server.hiveserver2-http-frontend.total-basic-auth-success impala.thrift-server.hiveserver2-http-frontend.total-basic-auth-failure which track the number of successful and failed attempts at using Basic auth to authentication to the hiveserver2 http server. A followup patch will add similar metrics for SASL attempts on the beeswax and hiveserver2 binary servers. Testing: - Modified existing HTTP auth test to check that the produced metrics are correct. Change-Id: I6d1b1bcc093e4b00802bc108ff4d8d4e2cdfd88f --- M be/src/rpc/auth-provider.h M be/src/rpc/authentication.cc M be/src/rpc/thrift-server.cc M be/src/rpc/thrift-server.h M be/src/transport/THttpServer.cpp M be/src/transport/THttpServer.h M common/thrift/metrics.json M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java 8 files changed, 111 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/40/13640/3 -- To view, visit http://gerrit.cloudera.org:8080/13640 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6d1b1bcc093e4b00802bc108ff4d8d4e2cdfd88f Gerrit-Change-Number: 13640 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-8583: Add metrics for BASIC auth
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13640 ) Change subject: IMPALA-8583: Add metrics for BASIC auth .. Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/13640/1/be/src/rpc/auth-provider.h File be/src/rpc/auth-provider.h: http://gerrit.cloudera.org:8080/#/c/13640/1/be/src/rpc/auth-provider.h@33 PS1, Line 33: AuthProvid > this doesn't seem right Done http://gerrit.cloudera.org:8080/#/c/13640/1/be/src/transport/THttpServer.h File be/src/transport/THttpServer.h: http://gerrit.cloudera.org:8080/#/c/13640/1/be/src/transport/THttpServer.h@84 PS1, Line 84: THttpSer > no need for explicit on multi-arg ctor Done http://gerrit.cloudera.org:8080/#/c/13640/1/be/src/transport/THttpServer.cpp File be/src/transport/THttpServer.cpp: http://gerrit.cloudera.org:8080/#/c/13640/1/be/src/transport/THttpServer.cpp@152 PS1, Line 152: total_basic_auth_success_->Increment(1); > semi-related to this patch: do we do any logging of the incorrect login att Yes, we log a warning with the username and ldap error on all failed attempts, as well as having a variety of other logging around different sasl/kerberos errors, and some vlog-ing on successful attempts. http://gerrit.cloudera.org:8080/#/c/13640/1/common/thrift/metrics.json File common/thrift/metrics.json: http://gerrit.cloudera.org:8080/#/c/13640/1/common/thrift/metrics.json@945 PS1, Line 945: Basic (use > in the long description perhaps we should say Basic (username/password) Aut Done http://gerrit.cloudera.org:8080/#/c/13640/1/common/thrift/metrics.json@949 PS1, Line 949: Basic > nit: BASIC isn't an acronym so I think it should just be capitalized as "Ba Done http://gerrit.cloudera.org:8080/#/c/13640/1/common/thrift/metrics.json@955 PS1, Line 955: auth > similar to above, maybe say something like "due to an invalid username and Done -- To view, visit http://gerrit.cloudera.org:8080/13640 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6d1b1bcc093e4b00802bc108ff4d8d4e2cdfd88f Gerrit-Change-Number: 13640 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 13 Jun 2019 23:24:42 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8583: Add metrics for BASIC auth
Hello Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13640 to look at the new patch set (#2). Change subject: IMPALA-8583: Add metrics for BASIC auth .. IMPALA-8583: Add metrics for BASIC auth The patch adds two metrics: impala.thrift-server.hiveserver2-http-frontend.total-basic-auth-success impala.thrift-server.hiveserver2-http-frontend.total-basic-auth-failure which track the number of successful and failed attempts at using BASIC auth to authentication to the hiveserver2 http server. A followup patch will add similar metrics for SASL attempts on the beeswax and hiveserver2 binary servers. Testing: - Modified existing HTTP auth test to check that the produced metrics are correct. Change-Id: I6d1b1bcc093e4b00802bc108ff4d8d4e2cdfd88f --- M be/src/rpc/auth-provider.h M be/src/rpc/authentication.cc M be/src/rpc/thrift-server.cc M be/src/rpc/thrift-server.h M be/src/transport/THttpServer.cpp M be/src/transport/THttpServer.h M common/thrift/metrics.json M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java 8 files changed, 111 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/40/13640/2 -- To view, visit http://gerrit.cloudera.org:8080/13640 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I6d1b1bcc093e4b00802bc108ff4d8d4e2cdfd88f Gerrit-Change-Number: 13640 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-8583: Add metrics for BASIC auth
Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13640 Change subject: IMPALA-8583: Add metrics for BASIC auth .. IMPALA-8583: Add metrics for BASIC auth The patch adds two metrics: impala.thrift-server.hiveserver2-http-frontend.total-basic-auth-success impala.thrift-server.hiveserver2-http-frontend.total-basic-auth-failure which track the number of successful and failed attempts at using BASIC auth to authentication to the hiveserver2 http server. A followup patch will add similar metrics for SASL attempts on the beeswax and hiveserver2 binary servers. Testing: - Modified existing HTTP auth test to check that the produced metrics are correct. Change-Id: I6d1b1bcc093e4b00802bc108ff4d8d4e2cdfd88f --- M be/src/rpc/auth-provider.h M be/src/rpc/authentication.cc M be/src/rpc/thrift-server.cc M be/src/rpc/thrift-server.h M be/src/transport/THttpServer.cpp M be/src/transport/THttpServer.h M common/thrift/metrics.json M fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java 8 files changed, 113 insertions(+), 22 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/40/13640/1 -- To view, visit http://gerrit.cloudera.org:8080/13640 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I6d1b1bcc093e4b00802bc108ff4d8d4e2cdfd88f Gerrit-Change-Number: 13640 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall
[Impala-ASF-CR] IMPALA-8629: (part 2) Adjust new KuduStorageHandler package
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13541 ) Change subject: IMPALA-8629: (part 2) Adjust new KuduStorageHandler package .. Patch Set 9: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13541 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4d0c505643247498383472704a37d27c9e1ce473 Gerrit-Change-Number: 13541 Gerrit-PatchSet: 9 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 12 Jun 2019 20:25:50 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8629: (part 2) Adjust new KuduStorageHandler package
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13541 ) Change subject: IMPALA-8629: (part 2) Adjust new KuduStorageHandler package .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13541 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4d0c505643247498383472704a37d27c9e1ce473 Gerrit-Change-Number: 13541 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 12 Jun 2019 17:32:31 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7467: Ported ExecQueryFInstances over to krpc
Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13583 Change subject: IMPALA-7467: Ported ExecQueryFInstances over to krpc .. IMPALA-7467: Ported ExecQueryFInstances over to krpc This patch ports the ExecQuerryFInstances rpc to use KRPC. The parameters for this call contain a huge number of Thrift structs (eg. everything related to TPlanNode and TExpr), so to avoid converting all of these to protobuf and the resulting effect that would have on the FE and catalog, this patch stores most of the parameters in a sidecar (in particular the TQueryCtx, TPlanFragmentCtx's, and TPlanFragmentInstanceCtx's). Testing: - Passed a full exhaustive run on the minicluster. Set up a ten node cluster with tpch 500: - Ran perf tests: 3 iterations per tpch query, 4 concurrent streams, no perf change. - Ran the stress test for 1000 queries, passed. Change-Id: Id3f1c6099109bd8e5361188005a7d0e892147570 --- M be/src/runtime/backend-client.h M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/query-exec-mgr.cc M be/src/runtime/query-exec-mgr.h M be/src/runtime/query-state.cc M be/src/runtime/query-state.h M be/src/runtime/test-env.cc M be/src/service/control-service.cc M be/src/service/control-service.h M be/src/service/impala-internal-service.cc M be/src/service/impala-internal-service.h M common/protobuf/control_service.proto M common/thrift/ImpalaInternalService.thrift M tests/custom_cluster/test_rpc_timeout.py 15 files changed, 247 insertions(+), 171 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/83/13583/1 -- To view, visit http://gerrit.cloudera.org:8080/13583 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Id3f1c6099109bd8e5361188005a7d0e892147570 Gerrit-Change-Number: 13583 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall
[Impala-ASF-CR] IMPALA-8623: Expose HS2 HTTP port in containers
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13520 ) Change subject: IMPALA-8623: Expose HS2 HTTP port in containers .. Patch Set 2: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/13520/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13520/2//COMMIT_MSG@10 PS2, Line 10: JDBC HTTP tests are not currently working. Once IMPALA-8626 is fixed, : they should exercise this. outdated -- To view, visit http://gerrit.cloudera.org:8080/13520 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iece20bc134fa5867f18b166cee2a2f75b21f9f36 Gerrit-Change-Number: 13520 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 10 Jun 2019 17:21:00 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Add option to set driver in ImpalaJdbcDriver
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13514 ) Change subject: Add option to set driver in ImpalaJdbcDriver .. Patch Set 4: Code-Review+2 gvo failure was unrelated -- To view, visit http://gerrit.cloudera.org:8080/13514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie6c8cec2f61e76b7c8321c954eef830778d8a8ee Gerrit-Change-Number: 13514 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 10 Jun 2019 16:24:09 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8623: Expose HS2 HTTP port in containers
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13520 ) Change subject: IMPALA-8623: Expose HS2 HTTP port in containers .. Patch Set 2: Did you still want to get this in, given the conclusion we reached on IMPALA-8626? -- To view, visit http://gerrit.cloudera.org:8080/13520 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iece20bc134fa5867f18b166cee2a2f75b21f9f36 Gerrit-Change-Number: 13520 Gerrit-PatchSet: 2 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Mon, 10 Jun 2019 16:13:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8629: (part 1) Add temp KuduStorageHandler
Thomas Marshall has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13561 ) Change subject: IMPALA-8629: (part 1) Add temp KuduStorageHandler .. IMPALA-8629: (part 1) Add temp KuduStorageHandler This patch adds a temporary KuduStorageHandler so that the Kudu project can change its handler without breaking the integration. It also disabled any tests that depend on the specific handler. A follow up patch will remove the TEMP_KUDU_STORAGE_HANDLER. adjust the KUDU_STORAGE_HANDLER value to be the final value, and re-enable the tests. Change-Id: Ic9982466699818390fa28efc5ea1aae75b11c12a Reviewed-on: http://gerrit.cloudera.org:8080/13561 Tested-by: Impala Public Jenkins Reviewed-by: Thomas Marshall --- M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java M tests/custom_cluster/test_kudu.py M tests/query_test/test_kudu.py 4 files changed, 24 insertions(+), 13 deletions(-) Approvals: Impala Public Jenkins: Verified Thomas Marshall: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/13561 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ic9982466699818390fa28efc5ea1aae75b11c12a Gerrit-Change-Number: 13561 Gerrit-PatchSet: 4 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8629: (part 1) Add temp KuduStorageHandler
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13561 ) Change subject: IMPALA-8629: (part 1) Add temp KuduStorageHandler .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13561 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic9982466699818390fa28efc5ea1aae75b11c12a Gerrit-Change-Number: 13561 Gerrit-PatchSet: 3 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Mon, 10 Jun 2019 16:12:39 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8635. Use local metastore URIs when checking for Kudu/HMS integration
Thomas Marshall has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13555 ) Change subject: IMPALA-8635. Use local metastore URIs when checking for Kudu/HMS integration .. IMPALA-8635. Use local metastore URIs when checking for Kudu/HMS integration The patch for IMPALA-8504 (part 2) (6bb404dc359) checks to see if Impala and Kudu are configured against the same metastore to determine if the HMS integration is enabled. However, instead of using its own metastore URI config, it uses the configuration stored on the remote HMS. This is error prone because it's not common for the HMS configuration to store its own URI. Instead, we should use our own config. This patch changes to using the local configuration for this purpose. More robust would be to use the HMS "UUID" support, since it's possible that Kudu and Impala are talking to different HMS instances sharing a backing DB, but that work is deferred to a later commit since it depends on Kudu-side changes. This commit doesn't add any tests, but fixes the existing tests when running against Hive 3, where the HMS server side uses a different configuration key for the metastore URIs. Change-Id: Id7a4c2cc0580f7c4dc5cfceed30b91e87c547612 Reviewed-on: http://gerrit.cloudera.org:8080/13555 Reviewed-by: Hao Hao Reviewed-by: Thomas Marshall Tested-by: Impala Public Jenkins --- M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java 3 files changed, 9 insertions(+), 17 deletions(-) Approvals: Hao Hao: Looks good to me, but someone else must approve Thomas Marshall: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/13555 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Id7a4c2cc0580f7c4dc5cfceed30b91e87c547612 Gerrit-Change-Number: 13555 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] Add option to set driver in ImpalaJdbcDriver
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13514 ) Change subject: Add option to set driver in ImpalaJdbcDriver .. Patch Set 2: Yeah, it was a draft, should be published now -- To view, visit http://gerrit.cloudera.org:8080/13514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie6c8cec2f61e76b7c8321c954eef830778d8a8ee Gerrit-Change-Number: 13514 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 07 Jun 2019 22:17:12 + Gerrit-HasComments: No
[Impala-ASF-CR] Add option to set driver in ImpalaJdbcDriver
Thomas Marshall has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/13514 ) Change subject: Add option to set driver in ImpalaJdbcDriver .. Add option to set driver in ImpalaJdbcDriver ImpalaJdbcDriver is a simple class that provides a wrapper around the Hive Jdbc driver for use in testing. This patch adds a '-d' parameter that takes a driver class and executes queries with that driver instead of the Hive driver, if its in the classpath. This makes it easy to use ./bin/run-jdbc-client.sh to test other Jdbc drivers, such as the proprietary Cloudera Impala driver. Change-Id: Ie6c8cec2f61e76b7c8321c954eef830778d8a8ee --- M fe/src/test/java/org/apache/impala/testutil/ImpalaJdbcClient.java 1 file changed, 26 insertions(+), 14 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/13514/2 -- To view, visit http://gerrit.cloudera.org:8080/13514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie6c8cec2f61e76b7c8321c954eef830778d8a8ee Gerrit-Change-Number: 13514 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8629: (part 1) Add temp KuduStorageHandler
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13561 ) Change subject: IMPALA-8629: (part 1) Add temp KuduStorageHandler .. Patch Set 2: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/13561/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java: http://gerrit.cloudera.org:8080/#/c/13561/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java@a444 PS2, Line 444: removed by mistake? -- To view, visit http://gerrit.cloudera.org:8080/13561 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic9982466699818390fa28efc5ea1aae75b11c12a Gerrit-Change-Number: 13561 Gerrit-PatchSet: 2 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 07 Jun 2019 21:30:26 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8635. Use local metastore URIs when checking for Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13555 ) Change subject: IMPALA-8635. Use local metastore URIs when checking for Kudu/HMS integration .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13555 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id7a4c2cc0580f7c4dc5cfceed30b91e87c547612 Gerrit-Change-Number: 13555 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 07 Jun 2019 17:45:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8506: Support RENAME TABLE statement with Kudu/HMS integration
Thomas Marshall has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13409 ) Change subject: IMPALA-8506: Support RENAME TABLE statement with Kudu/HMS integration .. IMPALA-8506: Support RENAME TABLE statement with Kudu/HMS integration This commit intends to support the actual handling of ALTER/RENAME TABLE DDL for managed Kudu tables with Kudu's integration with the Hive Metastore. However, currently Kudu is considered as the source of truth of the table schema, so when ALTER TABLE (ADD/DROP COLUMN/RANGE_PARTITION), Impala always directly alters/loads the Kudu tables. Thus, this commit only updates RENAME TABLE DDL, so that after the table is renamed in the Kudu, relies on Kudu to rename the table in the HMS. Change-Id: If7155e0b385b8ad81eda0a84277bc85171a88269 Reviewed-on: http://gerrit.cloudera.org:8080/13409 Reviewed-by: Grant Henke Reviewed-by: Alexey Serbin Reviewed-by: Thomas Marshall Tested-by: Impala Public Jenkins --- M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test A testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test M tests/custom_cluster/test_kudu.py M tests/query_test/test_kudu.py 5 files changed, 679 insertions(+), 16 deletions(-) Approvals: Grant Henke: Looks good to me, but someone else must approve Alexey Serbin: Looks good to me, but someone else must approve Thomas Marshall: Looks good to me, approved Impala Public Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/13409 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: If7155e0b385b8ad81eda0a84277bc85171a88269 Gerrit-Change-Number: 13409 Gerrit-PatchSet: 6 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8506: Support RENAME TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13409 ) Change subject: IMPALA-8506: Support RENAME TABLE statement with Kudu/HMS integration .. Patch Set 5: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13409 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If7155e0b385b8ad81eda0a84277bc85171a88269 Gerrit-Change-Number: 13409 Gerrit-PatchSet: 5 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 06 Jun 2019 20:37:14 + Gerrit-HasComments: No
[Impala-ASF-CR] Fix integration of kudu-hive.jar
Thomas Marshall has abandoned this change. ( http://gerrit.cloudera.org:8080/13544 ) Change subject: Fix integration of kudu-hive.jar .. Abandoned pushed by mistake -- To view, visit http://gerrit.cloudera.org:8080/13544 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: Ia874efa63d9114716da3b2ac8921e78b518bdae5 Gerrit-Change-Number: 13544 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall
[Impala-ASF-CR] Fix integration of kudu-hive.jar
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13542 ) Change subject: Fix integration of kudu-hive.jar .. Patch Set 4: Code-Review+2 carrying forward -- To view, visit http://gerrit.cloudera.org:8080/13542 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4ac79c3e9b8625ba54145dba23c69fd5117f35c7 Gerrit-Change-Number: 13542 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 06 Jun 2019 20:33:07 + Gerrit-HasComments: No
[Impala-ASF-CR] Fix integration of kudu-hive.jar
Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13544 Change subject: Fix integration of kudu-hive.jar .. Fix integration of kudu-hive.jar IMPALA-8503 added downloading kudu-hive.jar and adding it to HADOOP_CLASSPATH in run-hive-server.sh to allow the Hive Metastore to start with Kudu's HMS plugin. There are two problems with this that are fixed by this patch: - Previously, we fully specify the expected jar filename based on the value of IMPALA_KUDU_JAVA_VERSION when adding it to HADOOP_CLASSPATH but this is overly restrictive for users who may wish to override this value in impala-config-branch.sh to build their own branch with a different version of the kudu-hive.jar This patch relaxes this restriction by adding any jar containing the string kudu-hive in IMPALA_KUDU_JAVA_HOME to HADOOP_CLASSPATH - In bootstrap_toolchain, we don't download a package if its directory already exists. Since the 'kudu' and 'kudu-java' packages download to the same directory, this led to a race condition where 'kudu-java' might not be downloaded if 'kudu' had already been unpacked when it started. This patch fixes this by inspecting the contents of the Kudu package directory to look for specific files expected for each Kudu package. Change-Id: Ia874efa63d9114716da3b2ac8921e78b518bdae5 --- M bin/bootstrap_toolchain.py M bin/impala-config.sh M testdata/bin/run-hive-server.sh 3 files changed, 21 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/44/13544/1 -- To view, visit http://gerrit.cloudera.org:8080/13544 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ia874efa63d9114716da3b2ac8921e78b518bdae5 Gerrit-Change-Number: 13544 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall
[Impala-ASF-CR] Fix integration of kudu-hive.jar
Hello Lars Volker, Hao Hao, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13542 to look at the new patch set (#4). Change subject: Fix integration of kudu-hive.jar .. Fix integration of kudu-hive.jar IMPALA-8503 added downloading kudu-hive.jar and adding it to HADOOP_CLASSPATH in run-hive-server.sh to allow the Hive Metastore to start with Kudu's HMS plugin. There are two problems with this that are fixed by this patch: - Previously, we fully specify the expected jar filename based on the value of IMPALA_KUDU_JAVA_VERSION when adding it to HADOOP_CLASSPATH but this is overly restrictive for users who may wish to override this value in impala-config-branch.sh to build their own branch with a different version of the kudu-hive.jar This patch relaxes this restriction by adding any jar containing the string kudu-hive in IMPALA_KUDU_JAVA_HOME to HADOOP_CLASSPATH - In bootstrap_toolchain, we don't download a package if its directory already exists. Since the 'kudu' and 'kudu-java' packages download to the same directory, this led to a race condition where 'kudu-java' might not be downloaded if 'kudu' had already been unpacked when it started. This patch fixes this by inspecting the contents of the Kudu package directory to look for specific files expected for each Kudu package. Change-Id: I4ac79c3e9b8625ba54145dba23c69fd5117f35c7 --- M bin/bootstrap_toolchain.py M bin/impala-config.sh M testdata/bin/run-hive-server.sh 3 files changed, 21 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/13542/4 -- To view, visit http://gerrit.cloudera.org:8080/13542 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4ac79c3e9b8625ba54145dba23c69fd5117f35c7 Gerrit-Change-Number: 13542 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] Fix integration of kudu-hive.jar
Hello Lars Volker, Hao Hao, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13542 to look at the new patch set (#3). Change subject: Fix integration of kudu-hive.jar .. Fix integration of kudu-hive.jar IMPALA-8503 added downloading kudu-hive.jar and adding it to HADOOP_CLASSPATH in run-hive-server.sh to allow the Hive Metastore to start with Kudu's HMS plugin. There are two problems with this that are fixed by this patch: - Previously, we fully specify the expected jar filename based on the value of IMPALA_KUDU_JAVA_VERSION when adding it to HADOOP_CLASSPATH but this is overly restrictive for users who may wish to override this value in impala-config-branch.sh to build their own branch with a different version of the kudu-hive.jar This patch relaxes this restriction by adding any jar containing the string kudu-hive in IMPALA_KUDU_JAVA_HOME to HADOOP_CLASSPATH - In bootstrap_toolchain, we don't download a package if its directory already exists. Since the 'kudu' and 'kudu-java' packages download to the same directory, this led to a race condition where 'kudu-java' might not be downloaded if 'kudu' had already been unpacked when it started. This patch fixes this by inspecting the contents of the Kudu package directory to look for specific files expected for each Kudu package. Change-Id: I4ac79c3e9b8625ba54145dba23c69fd5117f35c7 --- M bin/bootstrap_toolchain.py M bin/impala-config.sh M testdata/bin/run-hive-server.sh 3 files changed, 19 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/13542/3 -- To view, visit http://gerrit.cloudera.org:8080/13542 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4ac79c3e9b8625ba54145dba23c69fd5117f35c7 Gerrit-Change-Number: 13542 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Lars Volker
[Impala-ASF-CR] Fix integration of kudu-hive.jar
Thomas Marshall has posted comments on this change. (
http://gerrit.cloudera.org:8080/13542 )
Change subject: Fix integration of kudu-hive.jar
..
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13542/2/bin/bootstrap_toolchain.py
File bin/bootstrap_toolchain.py:
http://gerrit.cloudera.org:8080/#/c/13542/2/bin/bootstrap_toolchain.py@433
PS2, Line 433: debug
> does this affect release builds?
Both the 'debug' and the 'release' directories will be present regardless of
the build type because we always just unpack and keep everything in the tarball.
http://gerrit.cloudera.org:8080/#/c/13542/2/testdata/bin/run-hive-server.sh
File testdata/bin/run-hive-server.sh:
http://gerrit.cloudera.org:8080/#/c/13542/2/testdata/bin/run-hive-server.sh@105
PS2, Line 105: ${IMPALA_KUDU_JAVA_HOME}/*
> I think this could just be
Done
--
To view, visit http://gerrit.cloudera.org:8080/13542
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4ac79c3e9b8625ba54145dba23c69fd5117f35c7
Gerrit-Change-Number: 13542
Gerrit-PatchSet: 3
Gerrit-Owner: Thomas Marshall
Gerrit-Reviewer: Csaba Ringhofer
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Thu, 06 Jun 2019 20:24:54 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] Fix integration of kudu-hive.jar
Hello Hao Hao, Csaba Ringhofer, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13542 to look at the new patch set (#2). Change subject: Fix integration of kudu-hive.jar .. Fix integration of kudu-hive.jar IMPALA-8503 added downloading kudu-hive.jar and adding it to HADOOP_CLASSPATH in run-hive-server.sh to allow the Hive Metastore to start with Kudu's HMS plugin. There are two problems with this that are fixed by this patch: - Previously, we fully specify the expected jar filename based on the value of IMPALA_KUDU_JAVA_VERSION when adding it to HADOOP_CLASSPATH but this is overly restrictive for users who may wish to override this value in impala-config-branch.sh to build their own branch with a different version of the kudu-hive.jar This patch relaxes this restriction by adding any jar containing the string kudu-hive in IMPALA_KUDU_JAVA_HOME to HADOOP_CLASSPATH - In bootstrap_toolchain, we don't download a package if its directory already exists. Since the 'kudu' and 'kudu-java' packages download to the same directory, this led to a race condition where 'kudu-java' might not be downloaded if 'kudu' had already been unpacked when it started. This patch fixes this by inspecting the contents of the Kudu package directory to look for specific files expected for each Kudu package. Change-Id: I4ac79c3e9b8625ba54145dba23c69fd5117f35c7 --- M bin/bootstrap_toolchain.py M bin/impala-config.sh M testdata/bin/run-hive-server.sh 3 files changed, 23 insertions(+), 5 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/13542/2 -- To view, visit http://gerrit.cloudera.org:8080/13542 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I4ac79c3e9b8625ba54145dba23c69fd5117f35c7 Gerrit-Change-Number: 13542 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins
[Impala-ASF-CR] Improve adding kudu-hive.jar to HADOOP CLASSPATH
Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13542 Change subject: Improve adding kudu-hive.jar to HADOOP_CLASSPATH .. Improve adding kudu-hive.jar to HADOOP_CLASSPATH IMPALA-8503 added kudu-hive.jar to HADOOP_CLASSPATH in run-hive-server.sh to allow the Hive Metastore to start with Kudu's HMS plugin. It accomplished this by fully specify the expected jar filename based on the value of IMPALA_KUDU_JAVA_VERSION, but this is overly restrictive for users who may wish to override this value in impala-config-branch.sh to build their own branch with a different version of the kudu-hive.jar This patch relaxes this restriction by adding any jar containing the string kudu-hive in IMPALA_KUDU_HOME to HADOOP_CLASSPATH Change-Id: I4ac79c3e9b8625ba54145dba23c69fd5117f35c7 --- M testdata/bin/run-hive-server.sh 1 file changed, 7 insertions(+), 3 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/13542/1 -- To view, visit http://gerrit.cloudera.org:8080/13542 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I4ac79c3e9b8625ba54145dba23c69fd5117f35c7 Gerrit-Change-Number: 13542 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall
[Impala-ASF-CR] IMPALA-8506: Support RENAME TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13409 ) Change subject: IMPALA-8506: Support RENAME TABLE statement with Kudu/HMS integration .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/13409/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/13409/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2590 PS4, Line 2590: boolean isKuduHmsIntegrationEnabled = false; I guess this works because if isManagedKuduTable=false and we don't actually set this value correctly then the if below will be true without evaluating "!isKuduHmsIntegrationEnabled"? Seems too complicated. My thinking was to just call isKuduHmsIntegrationEnabled() here, or if you want to avoid making the call unnecessarily in the case of external tables, maybe make this something like 'boolean shouldUpdateHMS = true' and then set 'shouldUpdateHMS = !isKuduHmsIntegrationEnabled()" in the "if (isManagedKuduTable)" block. -- To view, visit http://gerrit.cloudera.org:8080/13409 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If7155e0b385b8ad81eda0a84277bc85171a88269 Gerrit-Change-Number: 13409 Gerrit-PatchSet: 4 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 05 Jun 2019 22:42:08 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8506: Support RENAME TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13409 ) Change subject: IMPALA-8506: Support RENAME TABLE statement with Kudu/HMS integration .. Patch Set 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/13409/3/testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test File testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test: http://gerrit.cloudera.org:8080/#/c/13409/3/testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test@1 PS3, Line 1: # Test cases on Kudu tables that only expected to be ran with HMS integration enabled. > Please note how this file differs from kudu_alter.test and that they will b Also, please add an equivalent note at the top of kudu_alter.test and say that changes that are made to one file should be reflected in the other. -- To view, visit http://gerrit.cloudera.org:8080/13409 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If7155e0b385b8ad81eda0a84277bc85171a88269 Gerrit-Change-Number: 13409 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 05 Jun 2019 18:05:38 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8506: Support RENAME TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13409 ) Change subject: IMPALA-8506: Support RENAME TABLE statement with Kudu/HMS integration .. Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/13409/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/13409/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2588 PS3, Line 2588: isKuduHmsIntegrationEnabled(msTbl) 'boolean isKuduHmsIntegrationEnabled' so you can reuse it below? http://gerrit.cloudera.org:8080/#/c/13409/3/testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test File testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test: http://gerrit.cloudera.org:8080/#/c/13409/3/testdata/workloads/functional-query/queries/QueryTest/kudu_hms_alter.test@1 PS3, Line 1: # Test cases on Kudu tables that only expected to be ran with HMS integration enabled. Please note how this file differs from kudu_alter.test and that they will be combined in IMPALA-8614 In addition to the table names being different (the 'impala::' is removed), why was the test case 'Hash partitions cannot be enumerated as range partitions' removed? http://gerrit.cloudera.org:8080/#/c/13409/3/tests/custom_cluster/test_kudu.py File tests/custom_cluster/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/13409/3/tests/custom_cluster/test_kudu.py@117 PS3, Line 117: class TestKuduHMSIntegration(CustomClusterTestSuite, KuduTestSuite): It occurs to me - because this is a subclass of CustomClusterTestSuite, it will automatically restart Impala for each test in setup_method(), but because you haven't specified any custom args for Impala it will just be restarting it repeatedly with default args for no reason, making the tests take longer. I don't think this needs to be fixed in this patch, but something to address in the followup testing patch. (IMPALA-8614) -- To view, visit http://gerrit.cloudera.org:8080/13409 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If7155e0b385b8ad81eda0a84277bc85171a88269 Gerrit-Change-Number: 13409 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 05 Jun 2019 18:02:24 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8507: Support DROP TABLE statement with Kudu/HMS integration
Thomas Marshall has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13400 ) Change subject: IMPALA-8507: Support DROP TABLE statement with Kudu/HMS integration .. IMPALA-8507: Support DROP TABLE statement with Kudu/HMS integration This commit supports the actual handling of DROP TABLE DDL for managed Kudu tables with Kudu's integration with the Hive Metastore. When Kudu/HMS integration is enabled, after the table is dropped in the Kudu, relies on Kudu to drop the table in the HMS. Change-Id: I6d3b93957cc66009ad7a67fc513be2068f156abc Reviewed-on: http://gerrit.cloudera.org:8080/13400 Tested-by: Impala Public Jenkins Reviewed-by: Grant Henke Reviewed-by: Thomas Marshall --- M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M tests/custom_cluster/test_kudu.py 2 files changed, 149 insertions(+), 41 deletions(-) Approvals: Impala Public Jenkins: Verified Grant Henke: Looks good to me, but someone else must approve Thomas Marshall: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/13400 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I6d3b93957cc66009ad7a67fc513be2068f156abc Gerrit-Change-Number: 13400 Gerrit-PatchSet: 9 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8507: Support DROP TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13400 ) Change subject: IMPALA-8507: Support DROP TABLE statement with Kudu/HMS integration .. Patch Set 8: Code-Review+2 When the verify job fails and you rebase and rerun it, its helpful if you post why it failed, eg. due to known issue IMPALA-8567 -- To view, visit http://gerrit.cloudera.org:8080/13400 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6d3b93957cc66009ad7a67fc513be2068f156abc Gerrit-Change-Number: 13400 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 05 Jun 2019 17:17:50 + Gerrit-HasComments: No
[Impala-ASF-CR] Add option to set driver in ImpalaJdbcDriver
Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13514 Change subject: Add option to set driver in ImpalaJdbcDriver .. Add option to set driver in ImpalaJdbcDriver ImpalaJdbcDriver is a simple class that provides a wrapper around the Hive Jdbc driver for use in testing. This patch adds a '-d' parameter that takes a driver class and executes queries with that driver instead of the Hive driver, if its in the classpath. This makes it easy to use ./bin/run-jdbc-client.sh to test other Jdbc drivers, such as the proprietary Cloudera Impala driver. Change-Id: Ie6c8cec2f61e76b7c8321c954eef830778d8a8ee --- M fe/src/test/java/org/apache/impala/testutil/ImpalaJdbcClient.java 1 file changed, 25 insertions(+), 13 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/14/13514/1 -- To view, visit http://gerrit.cloudera.org:8080/13514 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ie6c8cec2f61e76b7c8321c954eef830778d8a8ee Gerrit-Change-Number: 13514 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall
[Impala-ASF-CR] IMPALA-8505: Disallow setting Kudu table id in ALTER TABLE
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13465 ) Change subject: IMPALA-8505: Disallow setting Kudu table id in ALTER TABLE .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13465 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibc1f3f87054db5e28f72a5b54ea5b3c040c1bf22 Gerrit-Change-Number: 13465 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Tue, 04 Jun 2019 19:49:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8507: Support DROP TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13400 ) Change subject: IMPALA-8507: Support DROP TABLE statement with Kudu/HMS integration .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13400 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6d3b93957cc66009ad7a67fc513be2068f156abc Gerrit-Change-Number: 13400 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Tue, 04 Jun 2019 18:06:19 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504 (part 2): Support CREATE TABLE statement with Kudu/HMS integration
Thomas Marshall has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13375 ) Change subject: IMPALA-8504 (part 2): Support CREATE TABLE statement with Kudu/HMS integration .. IMPALA-8504 (part 2): Support CREATE TABLE statement with Kudu/HMS integration This commit supports the actual handling of CREATE TABLE DDL for managed Kudu tables when integration with Hive Metastore is enabled. When Kudu/HMS integration is enabled, for CREATE TABLE statement, Impala can rely on Kudu to create the table in the HMS. Change-Id: Icffe412395f47f5e07d97bad457020770cfa7502 Reviewed-on: http://gerrit.cloudera.org:8080/13375 Reviewed-by: Thomas Marshall Reviewed-by: Grant Henke Tested-by: Thomas Marshall --- M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/catalog/Table.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java M tests/common/custom_cluster_test_suite.py M tests/common/kudu_test_suite.py M tests/common/skip.py M tests/custom_cluster/test_kudu.py M tests/query_test/test_kudu.py 10 files changed, 306 insertions(+), 20 deletions(-) Approvals: Thomas Marshall: Looks good to me, approved; Verified Grant Henke: Looks good to me, but someone else must approve -- To view, visit http://gerrit.cloudera.org:8080/13375 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Icffe412395f47f5e07d97bad457020770cfa7502 Gerrit-Change-Number: 13375 Gerrit-PatchSet: 10 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8504 (part 2): Support CREATE TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13375 ) Change subject: IMPALA-8504 (part 2): Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 9: Verified+1 There haven't been any code changes since the verify job on patch set 7, so I'll go ahead and verify it (the more recent verify failure was because I aborted the job) -- To view, visit http://gerrit.cloudera.org:8080/13375 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icffe412395f47f5e07d97bad457020770cfa7502 Gerrit-Change-Number: 13375 Gerrit-PatchSet: 9 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Tue, 04 Jun 2019 17:36:57 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504 (part 2): Support CREATE TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13375 ) Change subject: IMPALA-8504 (part 2): Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13375 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icffe412395f47f5e07d97bad457020770cfa7502 Gerrit-Change-Number: 13375 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Tue, 04 Jun 2019 17:31:10 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8507: Support DROP TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13400 ) Change subject: IMPALA-8507: Support DROP TABLE statement with Kudu/HMS integration .. Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/13400/4/tests/custom_cluster/test_kudu.py File tests/custom_cluster/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/13400/4/tests/custom_cluster/test_kudu.py@117 PS4, Line 117: class TestKuduHMSIntegration(CustomClusterTestSuite, KuduTestSuite): Could you add a TODO here references the JIRA you filed and mentioning that a lot of these tests are close copies of tests from query_test/test_kudu.py -- To view, visit http://gerrit.cloudera.org:8080/13400 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6d3b93957cc66009ad7a67fc513be2068f156abc Gerrit-Change-Number: 13400 Gerrit-PatchSet: 4 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Mon, 03 Jun 2019 23:02:31 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8504 (part 2): Support CREATE TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13375 ) Change subject: IMPALA-8504 (part 2): Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 6: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13375 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icffe412395f47f5e07d97bad457020770cfa7502 Gerrit-Change-Number: 13375 Gerrit-PatchSet: 6 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Mon, 03 Jun 2019 22:22:43 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8603: remove confusing logging from custom cluster tests
Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13502 Change subject: IMPALA-8603: remove confusing logging from custom cluster tests .. IMPALA-8603: remove confusing logging from custom cluster tests IMPALA-1653 added explicit closing of the HS2 session that is created by ImpalaTestSuite during teardown. For the custom cluster tests, this sometimes fails, either because the impalad was shutdown and is no longer reachable or because it was restarted and the new impalad doesn't know about the session that was created before restart. Currently, when these errors occur we log them. However, this is confusing because several custom cluster tests now log errors even though they succeed. This patch changes this to simply ignore the errors. One concern is that a test may hit a genuine error here and if so we won't have the logging for it. If this happens, though, it should generally be possible for a dev to repro the error with the logging enabled. Change-Id: I96144fdbe6cc9bf0f01a9951420ee6f281fa6649 --- M tests/common/impala_connection.py 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/02/13502/1 -- To view, visit http://gerrit.cloudera.org:8080/13502 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I96144fdbe6cc9bf0f01a9951420ee6f281fa6649 Gerrit-Change-Number: 13502 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall
[Impala-ASF-CR] IMPALA-8504 (part 2): Support CREATE TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13375 ) Change subject: IMPALA-8504 (part 2): Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/13375/5/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/13375/5/tests/query_test/test_kudu.py@1130 PS5, Line 1130: @SkipIfKudu.hms_integration_enabled I still think that a lot of the tests in this file and elsewhere that have been skipped don't need to be - eg. I'm not sure why this one wouldn't work as is with hms integration enabled. It also occurs to me - as written, this 'skip' won't ever actually be triggered in Impala's normal test runs, as we'll always run these tests against the default minicluster config, which doesn't enable HMS integration. Of course, even in that case people may use these tests to run in other situations, eg. there have been efforts to run them against real clusters, so it still makes sense to include the 'skip' for any tests that genuinely won't ever work with hms integration turned on. I'm wondering though if it would be worth the effort and testing time to parameterize these tests so that they all run both with and without hms integration enabled, eg. by having custom_cluster/TestKuduHMSIntegration inherent from some of these classes? A lot of the tests in this file really won't be impacted by the integration (eg. the scan tests) and it would be basically wasted test time to run them in both configurations, but a lot of them are affected (eg. this test is one I would definitely want to know works with HMS integration turned on) One question I have is what's Kudu plan for how this evolves in future version - i.e. is the non-HMS integrated functionality going to be aggressively deprecated, such that Impala will probably want to just make hms integration turned on the default config in our minicluster, or is the plan to maintain both code paths for awhile? Anyways, I think this patch has fairly good coverage as is, and we can probably just file a JIRA to clean some of this up later. -- To view, visit http://gerrit.cloudera.org:8080/13375 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icffe412395f47f5e07d97bad457020770cfa7502 Gerrit-Change-Number: 13375 Gerrit-PatchSet: 5 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Mon, 03 Jun 2019 18:09:57 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8503: allow the Hive Metastore to start with kudu-hive plugin
Thomas Marshall has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13319 ) Change subject: IMPALA-8503: allow the Hive Metastore to start with kudu-hive plugin .. IMPALA-8503: allow the Hive Metastore to start with kudu-hive plugin This patch allows to start the Hive Metasotre with Kudu plugin which is required for enabling Kudu's integration with the HMS. The Kudu plugin is downloaded and extracted from native-toolchain S3 bucket. Change-Id: I4bd1488ced51840ec986d29ed371e26168abcc76 Reviewed-on: http://gerrit.cloudera.org:8080/13319 Tested-by: Impala Public Jenkins Reviewed-by: Thomas Marshall --- M bin/bootstrap_toolchain.py M bin/impala-config.sh M fe/src/test/resources/hive-site.xml.py M impala-parent/pom.xml M testdata/bin/run-hive-server.sh 5 files changed, 22 insertions(+), 5 deletions(-) Approvals: Impala Public Jenkins: Verified Thomas Marshall: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/13319 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I4bd1488ced51840ec986d29ed371e26168abcc76 Gerrit-Change-Number: 13319 Gerrit-PatchSet: 6 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8503: allow the Hive Metastore to start with kudu-hive plugin
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13319 ) Change subject: IMPALA-8503: allow the Hive Metastore to start with kudu-hive plugin .. Patch Set 5: Code-Review+2 Thanks! -- To view, visit http://gerrit.cloudera.org:8080/13319 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4bd1488ced51840ec986d29ed371e26168abcc76 Gerrit-Change-Number: 13319 Gerrit-PatchSet: 5 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Mon, 03 Jun 2019 17:17:32 + Gerrit-HasComments: No
[Impala-ASF-CR] Disable cumstom cluster/service FE tests on S3
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13500 ) Change subject: Disable cumstom cluster/service FE tests on S3 .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13500 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaa95fb1cef6349afafe3fd3677df74f7fa225245 Gerrit-Change-Number: 13500 Gerrit-PatchSet: 1 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 03 Jun 2019 17:16:01 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504 (part 2): Support CREATE TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13375 ) Change subject: IMPALA-8504 (part 2): Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 4: (6 comments) http://gerrit.cloudera.org:8080/#/c/13375/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java File fe/src/main/java/org/apache/impala/catalog/KuduTable.java: http://gerrit.cloudera.org:8080/#/c/13375/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@172 PS4, Line 172: org.apache.hadoop.hive.metastore.api.Table msTbl, : boolean isHMSIntegrationEnabled these can fit on the same line http://gerrit.cloudera.org:8080/#/c/13375/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/13375/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@1930 PS4, Line 1930: !KuduTable.isHMSIntegrationEnabledAndValidate(masterHosts, : hmsUris formatting - put everything after the '=' on the next line, indented by 4 http://gerrit.cloudera.org:8080/#/c/13375/4/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java File fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java: http://gerrit.cloudera.org:8080/#/c/13375/4/fe/src/main/java/org/apache/impala/util/MetaStoreUtil.java@87 PS4, Line 87: public static final String HIVE_METASTORE_URIS_KEY = : "hive.metastore.uris"; This can fit on 1 line http://gerrit.cloudera.org:8080/#/c/13375/4/tests/custom_cluster/test_kudu.py File tests/custom_cluster/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/13375/4/tests/custom_cluster/test_kudu.py@129 PS4, Line 129: manged typo http://gerrit.cloudera.org:8080/#/c/13375/4/tests/query_test/test_kudu.py File tests/query_test/test_kudu.py: http://gerrit.cloudera.org:8080/#/c/13375/4/tests/query_test/test_kudu.py@93 PS4, Line 93: @SkipIfKudu.hms_integration_enabled Why would a test like this, which is focused on scanning kudu tables, need to be skipped if hms integration is turned on? http://gerrit.cloudera.org:8080/#/c/13375/4/tests/query_test/test_kudu.py@175 PS4, Line 175: @SkipIfKudu.hms_integration_enabled I assume that for cases like these, where the table is modified outside of Impala, we expect that Impala will automatically pick up the changes and the error won't occur? -- To view, visit http://gerrit.cloudera.org:8080/13375 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icffe412395f47f5e07d97bad457020770cfa7502 Gerrit-Change-Number: 13375 Gerrit-PatchSet: 4 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 31 May 2019 23:38:25 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Thomas Marshall has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration This commit adds support for the syntax of CREATE TABLE (and CTAS) statements for managed Kudu tables with Kudu/HMS integration. A follow up patch will address the actual handling of CREATE TABLE statement with Kudu/HMS integration. For a managed table the syntax remains the same. However, the detailed changes includes: 1) Kudu table will always be created with the new Kudu storage handler 'org.apache.kudu.hive.KuduStorageHandler' even when Kudu/HMS integration is disabled. The legacy storage handler will be eventually deprecated. 2) When Kudu/HMS integration is enabled, the Kudu table underneath the managed HMS table will follow the naming convention 'db_name.table_name' instead of 'impala::db_name.table_name'. 3) Add 'kudu.table_id' table property to be used with Kudu/HMS integration. This commit also extracts Kudu-related DDL parsing and analyzing tests, so that they can be run with or without Kudu/HMS integration enabled. Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Reviewed-on: http://gerrit.cloudera.org:8080/13318 Tested-by: Impala Public Jenkins Reviewed-by: Thomas Marshall --- M bin/run-all-tests.sh M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java M fe/src/main/java/org/apache/impala/catalog/KuduTable.java M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java M fe/src/main/java/org/apache/impala/util/KuduUtil.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java A fe/src/test/java/org/apache/impala/analysis/AuditingKuduTest.java M fe/src/test/java/org/apache/impala/analysis/AuditingTest.java M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java A fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java A fe/src/test/java/org/apache/impala/customservice/KuduHMSIntegrationTest.java M testdata/cluster/node_templates/common/etc/init.d/kudu-master M tests/query_test/test_kudu.py 16 files changed, 920 insertions(+), 577 deletions(-) Approvals: Impala Public Jenkins: Verified Thomas Marshall: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 14 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 13: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 13 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 31 May 2019 23:33:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8503: allow the Hive Metastore to start with kudu-hive plugin
Thomas Marshall has posted comments on this change. (
http://gerrit.cloudera.org:8080/13319 )
Change subject: IMPALA-8503: allow the Hive Metastore to start with kudu-hive
plugin
..
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13319/3/bin/bootstrap_toolchain.py
File bin/bootstrap_toolchain.py:
http://gerrit.cloudera.org:8080/#/c/13319/3/bin/bootstrap_toolchain.py@571
PS3, Line 571: cdh_components += [Package("kudu-java")]
Could you add a comment here like:
Always download Kudu's jars regardless of USE_CDH_KUDU since they aren't
platform dependent and aren't packaged by the toolchain.
http://gerrit.cloudera.org:8080/#/c/13319/3/bin/impala-config.sh
File bin/impala-config.sh:
http://gerrit.cloudera.org:8080/#/c/13319/3/bin/impala-config.sh@670
PS3, Line 670: export
IMPALA_KUDU_JAVA_VERSION=${IMPALA_KUDU_VERSION-"1.10.0-cdh6.x-SNAPSHOT"}
I don't think this logic works - you'll get a weird result in the case that
USE_CDH_KUDU=false, since your java version will be getting set to the random
git hash above.
Could you just modify the existing env var KUDU_JAVA_VERSION on line 174 to
have IMPALA_ as a prefix? (and fix any place that var is used, I think the only
place is impala-parent/pom.xml)
Also, please do a full build with USE_CDH_KUDU=false set in order to confirm
that it works (you don't have to actually do this on a non-CDH platform, just
set the env variable, and also please delete your toolchain directory before
doing it and verify that the contents of cdh_components/kudu... are as
expected, i.e. the kudu-hive jar and not the binaries)
--
To view, visit http://gerrit.cloudera.org:8080/13319
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4bd1488ced51840ec986d29ed371e26168abcc76
Gerrit-Change-Number: 13319
Gerrit-PatchSet: 3
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Fri, 31 May 2019 23:03:11 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8502: Bump CDH BUILD NUMBER and Kudu version
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13481 ) Change subject: IMPALA-8502: Bump CDH_BUILD_NUMBER and Kudu version .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13481 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4c9c7d264f08454d9da8ad2d06ead643214ee23e Gerrit-Change-Number: 13481 Gerrit-PatchSet: 4 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 31 May 2019 22:51:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8503: allow the Hive Metastore to start with kudu-hive plugin
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13319 ) Change subject: IMPALA-8503: allow the Hive Metastore to start with kudu-hive plugin .. Patch Set 2: (1 comment) http://gerrit.cloudera.org:8080/#/c/13319/2/bin/bootstrap_toolchain.py File bin/bootstrap_toolchain.py: http://gerrit.cloudera.org:8080/#/c/13319/2/bin/bootstrap_toolchain.py@424 PS2, Line 424: addtional_file = None So as written this isn't going to work for non-cdh supported platforms (eg. ubuntu14). That's because 'kudu' won't be added to the list of cdh components on such platforms (see line 573). On those platforms, we download the platform-specific Kudu bits from the toolchain instead (see line 542), but we'll still want to download these jars from cdh_components because the toolchain doesn't produce the jars. How about, we have two kudu components - call the Java one just 'kudu' and always add it to cdh_components on line 563. Then have another one, call it 'kudu-c++' or similar, which gets adds to cdh_components on line 573, and then modify the 'if' on line 425 to check for that component name. -- To view, visit http://gerrit.cloudera.org:8080/13319 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4bd1488ced51840ec986d29ed371e26168abcc76 Gerrit-Change-Number: 13319 Gerrit-PatchSet: 2 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 31 May 2019 20:29:37 + Gerrit-HasComments: Yes
[native-toolchain-CR] Bump Kudu version to 84086fe
Thomas Marshall has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13479 ) Change subject: Bump Kudu version to 84086fe .. Bump Kudu version to 84086fe This pulls in changes needed for the Impala side of the Kudu/HMS integration work. Change-Id: Ie63a8baf1564f63d6ec9c0b953e2fa4119f51190 Reviewed-on: http://gerrit.cloudera.org:8080/13479 Reviewed-by: Tim Armstrong Tested-by: Thomas Marshall --- M buildall.sh 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: Tim Armstrong: Looks good to me, approved Thomas Marshall: Verified -- To view, visit http://gerrit.cloudera.org:8080/13479 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: Ie63a8baf1564f63d6ec9c0b953e2fa4119f51190 Gerrit-Change-Number: 13479 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[native-toolchain-CR] Bump Kudu version to 84086fe
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13479 ) Change subject: Bump Kudu version to 84086fe .. Patch Set 2: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/13479 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie63a8baf1564f63d6ec9c0b953e2fa4119f51190 Gerrit-Change-Number: 13479 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 31 May 2019 19:41:35 + Gerrit-HasComments: No
[native-toolchain-CR] Bump Kudu version to 84086fe
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13479 ) Change subject: Bump Kudu version to 84086fe .. Patch Set 2: see https://gerrit.cloudera.org/#/c/13481/ -- To view, visit http://gerrit.cloudera.org:8080/13479 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie63a8baf1564f63d6ec9c0b953e2fa4119f51190 Gerrit-Change-Number: 13479 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 31 May 2019 18:49:13 + Gerrit-HasComments: No
[native-toolchain-CR] Bump Kudu version to 84086fe
Thomas Marshall has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/13479 ) Change subject: Bump Kudu version to 84086fe .. Bump Kudu version to 84086fe This pulls in changes needed for the Impala side of the Kudu/HMS integration work. Change-Id: Ie63a8baf1564f63d6ec9c0b953e2fa4119f51190 --- M buildall.sh 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/79/13479/2 -- To view, visit http://gerrit.cloudera.org:8080/13479 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ie63a8baf1564f63d6ec9c0b953e2fa4119f51190 Gerrit-Change-Number: 13479 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall
[Impala-ASF-CR] IMPALA-8502: Bump CDH BUILD NUMBER and Kudu version
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13481 ) Change subject: IMPALA-8502: Bump CDH_BUILD_NUMBER and Kudu version .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13481 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4c9c7d264f08454d9da8ad2d06ead643214ee23e Gerrit-Change-Number: 13481 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 31 May 2019 18:39:57 + Gerrit-HasComments: No
[native-toolchain-CR] Bump Kudu version to 84086fe
Thomas Marshall has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13479 Change subject: Bump Kudu version to 84086fe .. Bump Kudu version to 84086fe Change-Id: Ie63a8baf1564f63d6ec9c0b953e2fa4119f51190 --- M buildall.sh 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/native-toolchain refs/changes/79/13479/1 -- To view, visit http://gerrit.cloudera.org:8080/13479 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: Ie63a8baf1564f63d6ec9c0b953e2fa4119f51190 Gerrit-Change-Number: 13479 Gerrit-PatchSet: 1 Gerrit-Owner: Thomas Marshall
[Impala-ASF-CR] IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13299 ) Change subject: IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint .. Patch Set 6: Code-Review+2 rebased on top of my other related change -- To view, visit http://gerrit.cloudera.org:8080/13299 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic5569ac62ef3af2868b5d0581f5029dac736b2ff Gerrit-Change-Number: 13299 Gerrit-PatchSet: 6 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Mike Yoder Gerrit-Reviewer: Sudhanshu Arora Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 30 May 2019 03:07:48 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint
Hello Bharath Vissapragada, Michael Ho, Sudhanshu Arora, Mike Yoder, Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13299 to look at the new patch set (#6). Change subject: IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint .. IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint This patch adds an additional hiveserver2 endpoint for clients to connect to that uses HTTP. The endpoint can be disabled by setting --hs2_http_port=0. HTTP(S) also works when external TLS is enabled using --ssl_server_certificate. Thrift's http transport is modified to support BASIC authentication via ldap. For convenience of developing and reviewing, this patch is based on another that copied THttpServer and THttpTransport into Impala's codebase. Kerberos authentication is not supported, so the http endpoint is turned off if Kerberos is enabled and LDAP isn't. TODO = - Fuzz test the http endpoint - Add tests for LDAP + HTTPS Testing === - Parameterized JdbcTest and LdapJdbcTest to work for HS2 + HTTP mode - Added LdapHS2Test, which directly calls into the Hiveserver2 interface using a thrift http client. Manual testing with Beeline client (from Apache Hive), which has builtin support to connect to HTTP(S) based HS2 compatible endpoints. Example -- HTTP mode: > start-impala-cluster.py > JDBC_URL="jdbc:hive2://localhost:/default;transportMode=http" > beeline -u "$JDBC_URL" -- HTTPS mode: > cd $IMPALA_HOME > SSL_ARGS="--ssl_client_ca_certificate=./be/src/testutil/server-cert.pem \ --ssl_server_certificate=./be/src/testutil/server-cert.pem \ --ssl_private_key=./be/src/testutil/server-key.pem --hostname=localhost" > start-impala-cluster.py --impalad_args="$SSL_ARGS" \ --catalogd_args="$SSL_ARGS" --state_store_args="$SSL_ARGS" - Create a local trust store using 'keytool' and import the certificate from server-cert.pem (./clientkeystore in the example). > JDBC_URL="jdbc:hive2://localhost:/default;ssl=true;sslTrustStore= \ ./clientkeystore;trustStorePassword=password;transportMode=http" > beeline -u "$JDBC_URL" -- BASIC Auth with LDAP: > LDAP_ARGS="--enable_ldap_auth --ldap_uri='ldap://...' \ --ldap_bind_pattern='...' --ldap_passwords_in_clear_ok" > start-impala-cluster.py --impalad_args="$LDAP_ARGS" > JDBC_URL="jdbc:hive2://localhost:28000/default;user=...;password=\ ...;transportMode=http" > beeline -u "$JDBC_URL" -- HTTPS mode with LDAP: > start-impala-cluster.py --impalad_args="$LDAP_ARGS $SSL_ARGS" \ --catalogd_args="$SSL_ARGS" --state_store_args="$SSL_ARGS" > JDBC_URL="jdbc:hive2://localhost:28000/default;user=...;password=\ ...;ssl=true;sslTrustStore=./clientkeystore;trustStorePassword=\ password;transportMode=http" > beeline -u "$JDBC_URL" Change-Id: Ic5569ac62ef3af2868b5d0581f5029dac736b2ff --- M be/src/rpc/auth-provider.h M be/src/rpc/authentication-test.cc M be/src/rpc/authentication.cc M be/src/rpc/thrift-server.cc M be/src/rpc/thrift-server.h M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/service/impalad-main.cc M be/src/testutil/in-process-servers.cc M be/src/testutil/in-process-servers.h M be/src/transport/THttpServer.cpp M be/src/transport/THttpServer.h M be/src/transport/THttpTransport.cpp M be/src/transport/THttpTransport.h M bin/start-impala-cluster.py M common/thrift/generate_error_codes.py M common/thrift/metrics.json A fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java M fe/src/test/java/org/apache/impala/service/JdbcTest.java M fe/src/test/java/org/apache/impala/service/JdbcTestBase.java M fe/src/test/java/org/apache/impala/testutil/ImpalaJdbcClient.java M fe/src/test/resources/users.ldif M tests/common/impala_cluster.py 24 files changed, 683 insertions(+), 147 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/13299/6 -- To view, visit http://gerrit.cloudera.org:8080/13299 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic5569ac62ef3af2868b5d0581f5029dac736b2ff Gerrit-Change-Number: 13299 Gerrit-PatchSet: 6 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Mike Yoder Gerrit-Reviewer: Sudhanshu Arora Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 8: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 8 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 29 May 2019 22:43:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1653: Don't close hiveserver2 session when connection is closed
Hello Michael Ho, Andrew Sherman, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13306 to look at the new patch set (#8). Change subject: IMPALA-1653: Don't close hiveserver2 session when connection is closed .. IMPALA-1653: Don't close hiveserver2 session when connection is closed Currently, when a client connection is closed, we always close any session started over that connection. This is a requirement for beeswax, which always ties sessions to connections, but it is not required for hiveserver2, which allows sessions to be used across connections with a session token. This patch changes this behavior so that hiveserver2 sessions are no longer closed when the corresponding connection is closed. One downside of this change is that clients may inadvertently leave sessions open indefinitely if they close their connection without calling CloseSession(), which can waste space on the coordinator. We already have a flag --idle_session_timeout, but this flag is off by default and sessions that hit this timeout are expired but not fully closed. Rather than changing the default idle session behavior, which could affect existing users, this patch mitigates this issue by adding a new flag: --disconnected_esession_timeout which is set to 1 hour by default. When a session has had no open connections for longer than this time, it will be closed and any associated queries will be unregistered. Testing: - Added e2e tests. Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78 --- M be/src/rpc/thrift-server.cc M be/src/rpc/thrift-server.h M be/src/service/impala-beeswax-server.cc M be/src/service/impala-hs2-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M common/thrift/generate_error_codes.py M tests/common/impala_connection.py A tests/custom_cluster/test_hs2.py M tests/hs2/test_hs2.py 10 files changed, 338 insertions(+), 105 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/13306/8 -- To view, visit http://gerrit.cloudera.org:8080/13306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78 Gerrit-Change-Number: 13306 Gerrit-PatchSet: 8 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-1653: Don't close hiveserver2 session when connection is closed
Thomas Marshall has posted comments on this change. (
http://gerrit.cloudera.org:8080/13306 )
Change subject: IMPALA-1653: Don't close hiveserver2 session when connection is
closed
..
Patch Set 8:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13306/5/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/13306/5/be/src/service/impala-server.cc@2127
PS5, Line 2127: {
> Doesn't seem necessary to tackle here - filing a JIRA seems worthwhile.
IMPALA-8597
http://gerrit.cloudera.org:8080/#/c/13306/7/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/13306/7/be/src/service/impala-server.cc@2155
PS7, Line 2155: }
> It would be good to do the multiplication in 64 bits to avoid the possibili
Done
http://gerrit.cloudera.org:8080/#/c/13306/7/be/src/service/impala-server.cc@2158
PS7, Line 2158: int64_t session_timeout_ms =
session_state->session_timeout * 1000;
> DCHECK_EQ or DCHECK_ENUM_EQ?
Done
http://gerrit.cloudera.org:8080/#/c/13306/7/be/src/service/impala-server.cc@2199
PS7, Line 2199:
> typo
Done
--
To view, visit http://gerrit.cloudera.org:8080/13306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78
Gerrit-Change-Number: 13306
Gerrit-PatchSet: 8
Gerrit-Owner: Thomas Marshall
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Thomas Marshall
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Wed, 29 May 2019 19:40:15 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. (
http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
..
Patch Set 7:
(5 comments)
Looks good, just a couple more nitpicks I think before this can go in
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@171
PS7, Line 171: Error getting the configuration on whether
I find this awkwardly worded, maybe 'Error determining if'
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@173
PS7, Line 173: Kudu error
unnecessary
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
File fe/src/test/java/org/apache/impala/analysis/AuditingTest.java:
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java@56
PS7, Line 56: new TAccessEvent("functional.alltypes",
TCatalogObjectType.TABLE, "SELECT")));
Probably better to leave off unrelated formatting changes like this. It makes
it more difficult to deal with stuff like backports
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java
File fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java:
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java@49
PS7, Line 49: public static int RestartMiniclusterComponent(String component,
This should be removed
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java
File fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java:
http://gerrit.cloudera.org:8080/#/c/13318/7/fe/src/test/java/org/apache/impala/customservice/CustomServiceRunner.java@32
PS7, Line 32: mini cluster
minicluster component
--
To view, visit http://gerrit.cloudera.org:8080/13318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c
Gerrit-Change-Number: 13318
Gerrit-PatchSet: 7
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Wed, 29 May 2019 19:37:24 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1653: Don't close hiveserver2 session when connection is closed
Hello Michael Ho, Andrew Sherman, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13306 to look at the new patch set (#9). Change subject: IMPALA-1653: Don't close hiveserver2 session when connection is closed .. IMPALA-1653: Don't close hiveserver2 session when connection is closed Currently, when a client connection is closed, we always close any session started over that connection. This is a requirement for beeswax, which always ties sessions to connections, but it is not required for hiveserver2, which allows sessions to be used across connections with a session token. This patch changes this behavior so that hiveserver2 sessions are no longer closed when the corresponding connection is closed. One downside of this change is that clients may inadvertently leave sessions open indefinitely if they close their connection without calling CloseSession(), which can waste space on the coordinator. We already have a flag --idle_session_timeout, but this flag is off by default and sessions that hit this timeout are expired but not fully closed. Rather than changing the default idle session behavior, which could affect existing users, this patch mitigates this issue by adding a new flag: --disconnected_esession_timeout which is set to 1 hour by default. When a session has had no open connections for longer than this time, it will be closed and any associated queries will be unregistered. Testing: - Added e2e tests. Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78 --- M be/src/rpc/thrift-server.cc M be/src/rpc/thrift-server.h M be/src/service/impala-beeswax-server.cc M be/src/service/impala-hs2-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M common/thrift/generate_error_codes.py M tests/common/impala_connection.py A tests/custom_cluster/test_hs2.py M tests/hs2/test_hs2.py 10 files changed, 322 insertions(+), 89 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/13306/9 -- To view, visit http://gerrit.cloudera.org:8080/13306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78 Gerrit-Change-Number: 13306 Gerrit-PatchSet: 9 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-1653: Don't close hiveserver2 session when connection is closed
Hello Michael Ho, Andrew Sherman, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13306 to look at the new patch set (#10). Change subject: IMPALA-1653: Don't close hiveserver2 session when connection is closed .. IMPALA-1653: Don't close hiveserver2 session when connection is closed Currently, when a client connection is closed, we always close any session started over that connection. This is a requirement for beeswax, which always ties sessions to connections, but it is not required for hiveserver2, which allows sessions to be used across connections with a session token. This patch changes this behavior so that hiveserver2 sessions are no longer closed when the corresponding connection is closed. One downside of this change is that clients may inadvertently leave sessions open indefinitely if they close their connection without calling CloseSession(), which can waste space on the coordinator. We already have a flag --idle_session_timeout, but this flag is off by default and sessions that hit this timeout are expired but not fully closed. Rather than changing the default idle session behavior, which could affect existing users, this patch mitigates this issue by adding a new flag: --disconnected_session_timeout which is set to 1 hour by default. When a session has had no open connections for longer than this time, it will be closed and any associated queries will be unregistered. Testing: - Added e2e tests. Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78 --- M be/src/rpc/thrift-server.cc M be/src/rpc/thrift-server.h M be/src/service/impala-beeswax-server.cc M be/src/service/impala-hs2-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M common/thrift/generate_error_codes.py M tests/common/impala_connection.py A tests/custom_cluster/test_hs2.py M tests/hs2/test_hs2.py 10 files changed, 330 insertions(+), 89 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/13306/10 -- To view, visit http://gerrit.cloudera.org:8080/13306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78 Gerrit-Change-Number: 13306 Gerrit-PatchSet: 10 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-1653: Don't close hiveserver2 session when connection is closed
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13306 ) Change subject: IMPALA-1653: Don't close hiveserver2 session when connection is closed .. Patch Set 10: (4 comments) http://gerrit.cloudera.org:8080/#/c/13306/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13306/9//COMMIT_MSG@27 PS9, Line 27: disconnected_session_timeout > typo Done http://gerrit.cloudera.org:8080/#/c/13306/8/be/src/rpc/thrift-server.h File be/src/rpc/thrift-server.h: http://gerrit.cloudera.org:8080/#/c/13306/8/be/src/rpc/thrift-server.h@113 PS8, Line 113: context > context Done http://gerrit.cloudera.org:8080/#/c/13306/9/be/src/service/impala-server.h File be/src/service/impala-server.h: http://gerrit.cloudera.org:8080/#/c/13306/9/be/src/service/impala-server.h@1030 PS9, Line 1030: // the first time this session has been used on this connection. > Can we put the body of this if() in a separate function in impala-hs2-serve Done http://gerrit.cloudera.org:8080/#/c/13306/9/tests/common/impala_connection.py File tests/common/impala_connection.py: http://gerrit.cloudera.org:8080/#/c/13306/9/tests/common/impala_connection.py@280 PS9, Line 280: # The session may no longer be valid if the impalad was restarted during the test. > LOG.exception() so we get the backtrace if needed for later debugging (I as Done -- To view, visit http://gerrit.cloudera.org:8080/13306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78 Gerrit-Change-Number: 13306 Gerrit-PatchSet: 10 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 29 May 2019 21:27:58 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8538 (part 1) Copied THttp(Server|Transport) from thrift-0.9.3
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13298 ) Change subject: IMPALA-8538 (part 1) Copied THttp(Server|Transport) from thrift-0.9.3 .. Patch Set 2: Code-Review+2 see: https://gerrit.cloudera.org/#/c/13299/ -- To view, visit http://gerrit.cloudera.org:8080/13298 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1916e17eaeb7854eb93c2415396f0ee0243e4e32 Gerrit-Change-Number: 13298 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Tue, 28 May 2019 23:48:49 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13299 ) Change subject: IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint .. Patch Set 5: Code-Review+2 (1 comment) Fixed the clang-tidy errors. Carrying the +2 forward http://gerrit.cloudera.org:8080/#/c/13299/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13299/4//COMMIT_MSG@22 PS4, Line 22: - Fuzz test the http endpoint > Mind filing a JIRA for this one? Since it's possible this will be exposed o IMPALA-8591 -- To view, visit http://gerrit.cloudera.org:8080/13299 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic5569ac62ef3af2868b5d0581f5029dac736b2ff Gerrit-Change-Number: 13299 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Mike Yoder Gerrit-Reviewer: Sudhanshu Arora Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 28 May 2019 18:48:56 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint
Hello Bharath Vissapragada, Michael Ho, Sudhanshu Arora, Mike Yoder, Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13299 to look at the new patch set (#5). Change subject: IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint .. IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint This patch adds an additional hiveserver2 endpoint for clients to connect to that uses HTTP. The endpoint can be disabled by setting --hs2_http_port=0. HTTP(S) also works when external TLS is enabled using --ssl_server_certificate. Thrift's http transport is modified to support BASIC authentication via ldap. For convenience of developing and reviewing, this patch is based on another that copied THttpServer and THttpTransport into Impala's codebase. Kerberos authentication is not supported, so the http endpoint is turned off if Kerberos is enabled and LDAP isn't. TODO = - Fuzz test the http endpoint - Add tests for LDAP + HTTPS Testing === - Parameterized JdbcTest and LdapJdbcTest to work for HS2 + HTTP mode - Added LdapHS2Test, which directly calls into the Hiveserver2 interface using a thrift http client. Manual testing with Beeline client (from Apache Hive), which has builtin support to connect to HTTP(S) based HS2 compatible endpoints. Example -- HTTP mode: > start-impala-cluster.py > JDBC_URL="jdbc:hive2://localhost:/default;transportMode=http" > beeline -u "$JDBC_URL" -- HTTPS mode: > cd $IMPALA_HOME > SSL_ARGS="--ssl_client_ca_certificate=./be/src/testutil/server-cert.pem \ --ssl_server_certificate=./be/src/testutil/server-cert.pem \ --ssl_private_key=./be/src/testutil/server-key.pem --hostname=localhost" > start-impala-cluster.py --impalad_args="$SSL_ARGS" \ --catalogd_args="$SSL_ARGS" --state_store_args="$SSL_ARGS" - Create a local trust store using 'keytool' and import the certificate from server-cert.pem (./clientkeystore in the example). > JDBC_URL="jdbc:hive2://localhost:/default;ssl=true;sslTrustStore= \ ./clientkeystore;trustStorePassword=password;transportMode=http" > beeline -u "$JDBC_URL" -- BASIC Auth with LDAP: > LDAP_ARGS="--enable_ldap_auth --ldap_uri='ldap://...' \ --ldap_bind_pattern='...' --ldap_passwords_in_clear_ok" > start-impala-cluster.py --impalad_args="$LDAP_ARGS" > JDBC_URL="jdbc:hive2://localhost:28000/default;user=...;password=\ ...;transportMode=http" > beeline -u "$JDBC_URL" -- HTTPS mode with LDAP: > start-impala-cluster.py --impalad_args="$LDAP_ARGS $SSL_ARGS" \ --catalogd_args="$SSL_ARGS" --state_store_args="$SSL_ARGS" > JDBC_URL="jdbc:hive2://localhost:28000/default;user=...;password=\ ...;ssl=true;sslTrustStore=./clientkeystore;trustStorePassword=\ password;transportMode=http" > beeline -u "$JDBC_URL" Change-Id: Ic5569ac62ef3af2868b5d0581f5029dac736b2ff --- M be/src/rpc/auth-provider.h M be/src/rpc/authentication-test.cc M be/src/rpc/authentication.cc M be/src/rpc/thrift-server.cc M be/src/rpc/thrift-server.h M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/service/impalad-main.cc M be/src/testutil/in-process-servers.cc M be/src/testutil/in-process-servers.h M be/src/transport/THttpServer.cpp M be/src/transport/THttpServer.h M be/src/transport/THttpTransport.cpp M be/src/transport/THttpTransport.h M bin/start-impala-cluster.py M common/thrift/generate_error_codes.py M common/thrift/metrics.json A fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java M fe/src/test/java/org/apache/impala/service/JdbcTest.java M fe/src/test/java/org/apache/impala/service/JdbcTestBase.java M fe/src/test/java/org/apache/impala/testutil/ImpalaJdbcClient.java M fe/src/test/resources/users.ldif M tests/common/impala_cluster.py 24 files changed, 673 insertions(+), 143 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/13299/5 -- To view, visit http://gerrit.cloudera.org:8080/13299 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic5569ac62ef3af2868b5d0581f5029dac736b2ff Gerrit-Change-Number: 13299 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Mike Yoder Gerrit-Reviewer: Sudhanshu Arora Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-8333: Remove Impala Shell warnings part 2
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13404 ) Change subject: IMPALA-8333: Remove Impala Shell warnings part 2 .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13404 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8ae7e068894da5981fc083e690051da268bfde4d Gerrit-Change-Number: 13404 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Tue, 28 May 2019 18:28:32 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13318 ) Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration .. Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/13318/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2579 PS4, Line 2579: false > nit: not sure if this is standard practice in the Impala codebase, but migh We do that sometimes, its definitely appropriate. Also, I know you already have a patch out to fix this, but could you leave a TODO saying that we need to properly handle HMS integration here? Its good practice to avoid forgetting things, and it lets the reviewers know that this was intentional. http://gerrit.cloudera.org:8080/#/c/13318/5/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java File fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java: http://gerrit.cloudera.org:8080/#/c/13318/5/fe/src/test/java/org/apache/impala/customcluster/CustomClusterRunner.java@29 PS5, Line 29: public Leaving off this 'public' was intentional to reduce the chances someone will try to use this outside this package (which would break the filtering on package name in run-all-tests.sh). Could you instead just move the function you added to an equivalent package-private utility class in the 'customservice' package? -- To view, visit http://gerrit.cloudera.org:8080/13318 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c Gerrit-Change-Number: 13318 Gerrit-PatchSet: 5 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Fri, 24 May 2019 21:51:06 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8570: Fix flakiness in test restart statestore query resilience
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13406 ) Change subject: IMPALA-8570: Fix flakiness in test_restart_statestore_query_resilience .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13406 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0a54b6e149c42b696c954b5240d6de61453bf7f9 Gerrit-Change-Number: 13406 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 24 May 2019 18:25:35 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-8503: add option to start Kudu cluster with HMS integration
Thomas Marshall has abandoned this change. ( http://gerrit.cloudera.org:8080/13248 ) Change subject: IMPALA-8503: add option to start Kudu cluster with HMS integration .. Abandoned see https://gerrit.cloudera.org/#/c/13318/ -- To view, visit http://gerrit.cloudera.org:8080/13248 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: abandon Gerrit-Change-Id: I734d14ede6a03ad52e820e38a1fbcbac0a40ede2 Gerrit-Change-Number: 13248 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall
[Impala-ASF-CR] IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint
Thomas Marshall has posted comments on this change. (
http://gerrit.cloudera.org:8080/13299 )
Change subject: IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server
endpoint
..
Patch Set 4:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/13299/3/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/13299/3/be/src/rpc/authentication.cc@202
PS3, Line 202: // Return: true on success, false otherwise
> this function can be made static, right?
Done
http://gerrit.cloudera.org:8080/#/c/13299/3/be/src/rpc/authentication.cc@872
PS3, Line 872: // TODO: add support for SPNEGO style of HTTP auth
> I don't think this logic quite makes sense. Just because Kerberos is enable
Right. What we should really be checking here is if kerberos and enabled and
ldap isn't. In that situation, the user's expectation is clearly that all
endpoints are protected by kerberos, but we don't have any way to do that for
the http server, so a user who wants to have just kerberos enabled would need
to disable the http server.
Rather than failing on startup if a user fails to add the startup flag
--hs2_http_port=0, I guess we should probably just automatically disable the
http server.
http://gerrit.cloudera.org:8080/#/c/13299/3/be/src/rpc/authentication.cc@950
PS3, Line 950: void SecureAuthProvider::SetupConnectionContext(
> this can be a const reference to the shared_ptr
Done
http://gerrit.cloudera.org:8080/#/c/13299/3/be/src/rpc/authentication.cc@956
PS3, Line 956: lServerTran
> consider down_cast<> from gutil/casts.h here, which does an RTTI type check
Done
http://gerrit.cloudera.org:8080/#/c/13299/3/be/src/rpc/authentication.cc@963
PS3, Line 963:
> same
Done
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/transport/THttpServer.cpp@79
PS2, Line 79: char* path = strchr(method, ' ');
> BTW, yea, I think an LDAP query per RPC is probably pretty bad considering
I filed IMPALA-8584
http://gerrit.cloudera.org:8080/#/c/13299/3/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:
http://gerrit.cloudera.org:8080/#/c/13299/3/be/src/transport/THttpServer.cpp@71
PS3, Line 71:
> RFC7230 says the whitespace between the : and the header value is optional:
Done
http://gerrit.cloudera.org:8080/#/c/13299/3/be/src/transport/THttpServer.cpp@75
PS3, Line 75:
> does this end up closing the connection immediately? If not, I'm worried ab
Done
http://gerrit.cloudera.org:8080/#/c/13299/3/common/thrift/metrics.json
File common/thrift/metrics.json:
http://gerrit.cloudera.org:8080/#/c/13299/3/common/thrift/metrics.json@904
PS3, Line 904: {
> nit: weird indentation
Done
http://gerrit.cloudera.org:8080/#/c/13299/3/common/thrift/metrics.json@909
PS3, Line 909: "label": "HiveServer2 HTTP API Active Connections",
> need to make these labels unique to HTTP (here and below)
Done
http://gerrit.cloudera.org:8080/#/c/13299/3/common/thrift/metrics.json@944
PS3, Line 944: {
> should we file a JIRA to add metrics for unauthorized conn attempts? or is
I filed IMPALA-8583
http://gerrit.cloudera.org:8080/#/c/13299/3/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java
File fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java:
http://gerrit.cloudera.org:8080/#/c/13299/3/fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java@102
PS3, Line 102: // Authenticate as 'Test1Ldap' with password '12345'
> worth adding a negative test with the wrong password, as well as various in
Done
--
To view, visit http://gerrit.cloudera.org:8080/13299
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic5569ac62ef3af2868b5d0581f5029dac736b2ff
Gerrit-Change-Number: 13299
Gerrit-PatchSet: 4
Gerrit-Owner: Thomas Marshall
Gerrit-Reviewer: Bharath Vissapragada
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Mike Yoder
Gerrit-Reviewer: Sudhanshu Arora
Gerrit-Reviewer: Thomas Marshall
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 24 May 2019 04:32:48 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint
Hello Bharath Vissapragada, Michael Ho, Sudhanshu Arora, Mike Yoder, Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13299 to look at the new patch set (#4). Change subject: IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint .. IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint This patch adds an additional hiveserver2 endpoint for clients to connect to that uses HTTP. The endpoint can be disabled by setting --hs2_http_port=0. HTTP(S) also works when external TLS is enabled using --ssl_server_certificate. Thrift's http transport is modified to support BASIC authentication via ldap. For convenience of developing and reviewing, this patch is based on another that copied THttpServer and THttpTransport into Impala's codebase. Kerberos authentication is not supported, so the http endpoint is turned off if Kerberos is enabled and LDAP isn't. TODO = - Fuzz test the http endpoint - Add tests for LDAP + HTTPS Testing === - Parameterized JdbcTest and LdapJdbcTest to work for HS2 + HTTP mode - Added LdapHS2Test, which directly calls into the Hiveserver2 interface using a thrift http client. Manual testing with Beeline client (from Apache Hive), which has builtin support to connect to HTTP(S) based HS2 compatible endpoints. Example -- HTTP mode: > start-impala-cluster.py > JDBC_URL="jdbc:hive2://localhost:/default;transportMode=http" > beeline -u "$JDBC_URL" -- HTTPS mode: > cd $IMPALA_HOME > SSL_ARGS="--ssl_client_ca_certificate=./be/src/testutil/server-cert.pem \ --ssl_server_certificate=./be/src/testutil/server-cert.pem \ --ssl_private_key=./be/src/testutil/server-key.pem --hostname=localhost" > start-impala-cluster.py --impalad_args="$SSL_ARGS" \ --catalogd_args="$SSL_ARGS" --state_store_args="$SSL_ARGS" - Create a local trust store using 'keytool' and import the certificate from server-cert.pem (./clientkeystore in the example). > JDBC_URL="jdbc:hive2://localhost:/default;ssl=true;sslTrustStore= \ ./clientkeystore;trustStorePassword=password;transportMode=http" > beeline -u "$JDBC_URL" -- BASIC Auth with LDAP: > LDAP_ARGS="--enable_ldap_auth --ldap_uri='ldap://...' \ --ldap_bind_pattern='...' --ldap_passwords_in_clear_ok" > start-impala-cluster.py --impalad_args="$LDAP_ARGS" > JDBC_URL="jdbc:hive2://localhost:28000/default;user=...;password=\ ...;transportMode=http" > beeline -u "$JDBC_URL" -- HTTPS mode with LDAP: > start-impala-cluster.py --impalad_args="$LDAP_ARGS $SSL_ARGS" \ --catalogd_args="$SSL_ARGS" --state_store_args="$SSL_ARGS" > JDBC_URL="jdbc:hive2://localhost:28000/default;user=...;password=\ ...;ssl=true;sslTrustStore=./clientkeystore;trustStorePassword=\ password;transportMode=http" > beeline -u "$JDBC_URL" Change-Id: Ic5569ac62ef3af2868b5d0581f5029dac736b2ff --- M be/src/rpc/auth-provider.h M be/src/rpc/authentication-test.cc M be/src/rpc/authentication.cc M be/src/rpc/thrift-server.cc M be/src/rpc/thrift-server.h M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/service/impalad-main.cc M be/src/testutil/in-process-servers.cc M be/src/testutil/in-process-servers.h M be/src/transport/THttpServer.cpp M be/src/transport/THttpServer.h M be/src/transport/THttpTransport.cpp M be/src/transport/THttpTransport.h M bin/start-impala-cluster.py M common/thrift/generate_error_codes.py M common/thrift/metrics.json A fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java M fe/src/test/java/org/apache/impala/service/JdbcTest.java M fe/src/test/java/org/apache/impala/service/JdbcTestBase.java M fe/src/test/java/org/apache/impala/testutil/ImpalaJdbcClient.java M fe/src/test/resources/users.ldif M tests/common/impala_cluster.py 24 files changed, 666 insertions(+), 136 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/13299/4 -- To view, visit http://gerrit.cloudera.org:8080/13299 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic5569ac62ef3af2868b5d0581f5029dac736b2ff Gerrit-Change-Number: 13299 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Mike Yoder Gerrit-Reviewer: Sudhanshu Arora Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Todd Lipcon
[Impala-ASF-CR] IMPALA-4271: Introduce the new Kudu storage handler
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13358 ) Change subject: IMPALA-4271: Introduce the new Kudu storage handler .. Patch Set 7: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13358 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75bcd5246005f4e35251aef9219f4d07eeb87dc6 Gerrit-Change-Number: 13358 Gerrit-PatchSet: 7 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 23 May 2019 21:10:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1653: Don't close hiveserver2 session when connection is closed
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13306 ) Change subject: IMPALA-1653: Don't close hiveserver2 session when connection is closed .. Patch Set 7: (1 comment) http://gerrit.cloudera.org:8080/#/c/13306/6/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/13306/6/be/src/service/impala-server.cc@2152 PS6, Line 2152: > line too long (103 > 90) Done -- To view, visit http://gerrit.cloudera.org:8080/13306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78 Gerrit-Change-Number: 13306 Gerrit-PatchSet: 7 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 23 May 2019 20:58:53 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1653: Don't close hiveserver2 session when connection is closed
Hello Michael Ho, Andrew Sherman, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13306 to look at the new patch set (#7). Change subject: IMPALA-1653: Don't close hiveserver2 session when connection is closed .. IMPALA-1653: Don't close hiveserver2 session when connection is closed Currently, when a client connection is closed, we always close any session started over that connection. This is a requirement for beeswax, which always ties sessions to connections, but it is not required for hiveserver2, which allows sessions to be used across connections with a session token. This patch changes this behavior so that hiveserver2 sessions are no longer closed when the corresponding connection is closed. One downside of this change is that clients may inadvertently leave sessions open indefinitely if they close their connection without calling CloseSession(), which can waste space on the coordinator. We already have a flag --idle_session_timeout, but this flag is off by default and sessions that hit this timeout are expired but not fully closed. Rather than changing the default idle session behavior, which could affect existing users, this patch mitigates this issue by adding a new flag: --disconnected_esession_timeout which is set to 1 hour by default. When a session has had no open connections for longer than this time, it will be closed and any associated queries will be unregistered. Testing: - Added e2e tests. Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78 --- M be/src/service/impala-hs2-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M common/thrift/generate_error_codes.py M tests/common/impala_connection.py A tests/custom_cluster/test_hs2.py M tests/hs2/test_hs2.py 7 files changed, 281 insertions(+), 61 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/13306/7 -- To view, visit http://gerrit.cloudera.org:8080/13306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78 Gerrit-Change-Number: 13306 Gerrit-PatchSet: 7 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8504: Introduce the new Kudu storage handler
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13358 ) Change subject: IMPALA-8504: Introduce the new Kudu storage handler .. Patch Set 6: If you'll address the previous comment about the JIRA references in the commit message, I think this is ready to go in -- To view, visit http://gerrit.cloudera.org:8080/13358 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I75bcd5246005f4e35251aef9219f4d07eeb87dc6 Gerrit-Change-Number: 13358 Gerrit-PatchSet: 6 Gerrit-Owner: Hao Hao Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Hao Hao Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 23 May 2019 20:21:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-1653: Don't close hiveserver2 session when connection is closed
Hello Michael Ho, Andrew Sherman, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13306 to look at the new patch set (#6). Change subject: IMPALA-1653: Don't close hiveserver2 session when connection is closed .. IMPALA-1653: Don't close hiveserver2 session when connection is closed Currently, when a client connection is closed, we always close any session started over that connection. This is a requirement for beeswax, which always ties sessions to connections, but it is not required for hiveserver2, which allows sessions to be used across connections with a session token. This patch changes this behavior so that hiveserver2 sessions are no longer closed when the corresponding connection is closed. One downside of this change is that clients may inadvertently leave sessions open indefinitely if they close their connection without calling CloseSession(), which can waste space on the coordinator. We already have a flag --idle_session_timeout, but this flag is off by default and sessions that hit this timeout are expired but not fully closed. Rather than changing the default idle session behavior, which could affect existing users, this patch mitigates this issue by adding a new flag: --disconnected_esession_timeout which is set to 1 hour by default. When a session has had no open connections for longer than this time, it will be closed and any associated queries will be unregistered. Testing: - Added e2e tests. Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78 --- M be/src/service/impala-hs2-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h M tests/common/impala_connection.py A tests/custom_cluster/test_hs2.py M tests/hs2/test_hs2.py 6 files changed, 264 insertions(+), 57 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/13306/6 -- To view, visit http://gerrit.cloudera.org:8080/13306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78 Gerrit-Change-Number: 13306 Gerrit-PatchSet: 6 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-1653: Don't close hiveserver2 session when connection is closed
Thomas Marshall has posted comments on this change. (
http://gerrit.cloudera.org:8080/13306 )
Change subject: IMPALA-1653: Don't close hiveserver2 session when connection is
closed
..
Patch Set 6:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/13306/5/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/13306/5/be/src/service/impala-server.cc@211
PS5, Line 211: DEFINE_int32(disconnected_session_timeout, 15 * 60, "The time,
in seconds, that a "
> I think we could set this lower. One data point is that hue sets the query
Done
http://gerrit.cloudera.org:8080/#/c/13306/5/be/src/service/impala-server.cc@1393
PS5, Line 1393: if (session_type == TSessionType::HIVESERVER2) {
> Probably worth documenting why this is necessary, i.e. we might be using th
Done
http://gerrit.cloudera.org:8080/#/c/13306/5/be/src/service/impala-server.cc@2053
PS5, Line 2053: ::ConnectionEnd(
> Maybe disconnected_sessions?
Done
http://gerrit.cloudera.org:8080/#/c/13306/5/be/src/service/impala-server.cc@2064
PS5, Line 2064: // We don't expect a large number of sessions per
connection, so we copy it, so that
> move() to avoid copying set?
Done
http://gerrit.cloudera.org:8080/#/c/13306/5/be/src/service/impala-server.cc@2085
PS5, Line 2085: }
> DCHECK_EQ
Done
http://gerrit.cloudera.org:8080/#/c/13306/5/be/src/service/impala-server.cc@2127
PS5, Line 2127: } else {
> Not your change, but I feel like this was a bit overengineered. If I unders
Yeah it looks like the original intention was to adaptively set the sleep
period to be equal to the smallest registered timeout divided by 2, hence using
a set to store all the individual timeout values.
That behavior resulted in IMPALA-5108, which was fixed by setting the timeout
to always be 1 second, but we didn't remove the set then.
I think it still makes sense to have a mechanism for not running this thread
when no session timeouts are set, since that's the default situation, but we
could do it more efficiently by just maintaining a count of registered
timeouts. I can take that on in this patch or file a JIRA for it.
http://gerrit.cloudera.org:8080/#/c/13306/5/be/src/service/impala-server.cc@2150
PS5, Line 2150: if (session_state->closed) continue;
> Why is this mutually exclusive with the idle check? I think if this hits th
Done
http://gerrit.cloudera.org:8080/#/c/13306/5/be/src/service/impala-server.cc@2192
PS5, Line 2192: }
> Missing an and/or?
Done
http://gerrit.cloudera.org:8080/#/c/13306/5/tests/custom_cluster/test_hs2.py
File tests/custom_cluster/test_hs2.py:
http://gerrit.cloudera.org:8080/#/c/13306/5/tests/custom_cluster/test_hs2.py@40
PS5, Line 40: def test_disconnected_session_timeout(self):
> Would be good to also test interaction with the idle timeout - i.e. if ther
Done
http://gerrit.cloudera.org:8080/#/c/13306/5/tests/hs2/test_hs2.py
File tests/hs2/test_hs2.py:
http://gerrit.cloudera.org:8080/#/c/13306/5/tests/hs2/test_hs2.py@621
PS5, Line 621: remains
> remains
Done
--
To view, visit http://gerrit.cloudera.org:8080/13306
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78
Gerrit-Change-Number: 13306
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Marshall
Gerrit-Reviewer: Andrew Sherman
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Thomas Marshall
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Thu, 23 May 2019 19:56:34 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8570: Fix flakiness in test restart statestore query resilience
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13406 ) Change subject: IMPALA-8570: Fix flakiness in test_restart_statestore_query_resilience .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/13406/1/tests/custom_cluster/test_restart_services.py File tests/custom_cluster/test_restart_services.py: http://gerrit.cloudera.org:8080/#/c/13406/1/tests/custom_cluster/test_restart_services.py@96 PS1, Line 96: @SkipIfEC.scheduling My concern with this is that its not very future-proof, i.e. we could conceivably have other jobs besides the ec job in the future that also suffer from this same issue. Is it possible to rewrite the test to not have the requirement for 3 backends? Or possibly to write some sort of more general SkipIfNonstandardMinicluster? Or at least grab the profile for the query and assert that it shows it being scheduled on 3 backends, so that we would get a clear error message if it didn't rather than just flakiness that someone has to go track down? -- To view, visit http://gerrit.cloudera.org:8080/13406 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0a54b6e149c42b696c954b5240d6de61453bf7f9 Gerrit-Change-Number: 13406 Gerrit-PatchSet: 1 Gerrit-Owner: Bikramjeet Vig Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Thu, 23 May 2019 16:51:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7290: part 2: Add HS2 support to Impala shell
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12884 ) Change subject: IMPALA-7290: part 2: Add HS2 support to Impala shell .. Patch Set 18: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/12884/16/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/12884/16/common/thrift/ImpalaService.thrift@545 PS16, Line 545: // Same as HS2 CloseOperation but can return additional information. > Weirdly the number of modified rows was returned in the operation handle: h Interesting. Well anyways, I think this is probably fine for the purposes of this patch. If we need better vanilla hs2 compatibility in the future for some reason we can always see about upstreaming some of this -- To view, visit http://gerrit.cloudera.org:8080/12884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6d5cc83d545aacc659523f29b1d6feed672e2a12 Gerrit-Change-Number: 12884 Gerrit-PatchSet: 18 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 22 May 2019 21:54:05 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint
Thomas Marshall has posted comments on this change. (
http://gerrit.cloudera.org:8080/13299 )
Change subject: IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server
endpoint
..
Patch Set 3:
(17 comments)
http://gerrit.cloudera.org:8080/#/c/13299/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13299/2//COMMIT_MSG@21
PS2, Line 21:
:
: TODO
> It seems to me like it would be easier to just copy-paste this, since the T
Sure. I checked, and the differences between this version and thrift master's
THttp* is quite minimal. Its certainly less work for me not to have to patch
native-toolchain.
I'm interested in trying to contribute this upstream, but I assume that process
will take awhile, so for this patch I'll go ahead and take the "[not for
review]" off of the patch that copies the files that this patch is based on.
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/rpc/authentication.cc
File be/src/rpc/authentication.cc:
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/rpc/authentication.cc@502
PS2, Line 502: LOG(ERROR) << "Failed to decode base64 auth string from: "
> for all of the log messages in this function, can we include the remote soc
Done
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/rpc/authentication.cc@505
PS2, Line 505: }
> stack allocating here seems quite dangerous without constraining the length
Done
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/rpc/authentication.cc@507
PS2, Line 507: if (colon == std::string::npos) {
> Looking at Base64Decode, it doesn't seem to null-terminate the output, but
Done
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/rpc/authentication.cc@521
PS2, Line 521:
> again I'd feel safer about the above code if we used C++ strings, like:
Done
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/rpc/authentication.cc@957
PS2, Line 957:
> this function is a bit weirdly named, since in the HTTP case, it isn't sett
Done
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/rpc/authentication.cc@977
PS2, Line 977: switch (underlying_transport_type) {
> can we just LOG(FATAL) here since it would be a coding bug?
Done
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/rpc/authentication.cc@998
PS2, Line 998: }
> same
Done
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/service/impala-server.cc@2424
PS2, Line 2424: ThriftServer* server;
> Maybe we should use '-1' to mean disable? port 0 usually means "use an ephe
The problem with that is that the equivalent flags, eg. for hs2_port and
beewax, use 0 to mean disabled. I'm not sure its a good idea either to have
inconsistent behavior between flags or to change the behavior of the existing
flags.
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/transport/THttpServer.h
File be/src/transport/THttpServer.h:
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/transport/THttpServer.h@71
PS2, Line 71: Wraps a transport i
> that's a bit of an odd choice of type instead of std::string
Done
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/transport/THttpServer.h@80
PS2, Line 80:
> explicit
Done
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/transport/THttpServer.cpp
File be/src/transport/THttpServer.cpp:
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/transport/THttpServer.cpp@68
PS2, Line 68: rwarded
> probably needs to be made case-insensitive (odd that x-forwarded-for is not
Done
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/transport/THttpServer.cpp@69
PS2, Line 69:
> probably need to also check that sz >= 7, otherwise we might read past the
Done
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/transport/THttpServer.cpp@76
PS2, Line 76: }
: authorized_ = true;
> is base64AuthString_ were a string you could just use a simple assignment h
Done
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/transport/THttpServer.cpp@79
PS2, Line 79: }
> I think it's worth considering this a bit carefully. It seems that the desi
As discussed, I went with a design where we set the username on the connection
context each time we authenticate and then check that the session username
matches the current connection username when doing operations. This works
because each connection corresponds to a single thread which will read some
headers and process the corresponding rpc one at a time.
One concern this leaves me with is that now we're hitting ldap on every rpc. If
that seems like a potential perf issue, I could add a list of already
authenticated base64 strings here and check that before calling the auth fn, or
I could add support for cookies, which is probably a better solution but I'm
not sure how much work it would be.
http://gerrit.cloudera.org:8080/#/c/13299/2/be/src/transport/THttpServer.cpp@193
[Impala-ASF-CR] IMPALA-8538 (part 1) Copied THttp(Server|Transport) from thrift-0.9.3
Hello Michael Ho, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13298 to look at the new patch set (#2). Change subject: IMPALA-8538 (part 1) Copied THttp(Server|Transport) from thrift-0.9.3 .. IMPALA-8538 (part 1) Copied THttp(Server|Transport) from thrift-0.9.3 This is a mechanical change that just copies several files over from thrift. This is for convenience in reviewing changes to these files, which have been submitted as a follow up patch. Change-Id: I1916e17eaeb7854eb93c2415396f0ee0243e4e32 --- M be/src/transport/CMakeLists.txt A be/src/transport/THttpServer.cpp A be/src/transport/THttpServer.h A be/src/transport/THttpTransport.cpp A be/src/transport/THttpTransport.h 5 files changed, 602 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/13298/2 -- To view, visit http://gerrit.cloudera.org:8080/13298 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I1916e17eaeb7854eb93c2415396f0ee0243e4e32 Gerrit-Change-Number: 13298 Gerrit-PatchSet: 2 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho
[Impala-ASF-CR] IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint
Hello Bharath Vissapragada, Michael Ho, Sudhanshu Arora, Mike Yoder, Todd Lipcon, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13299 to look at the new patch set (#3). Change subject: IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint .. IMPALA-8538: HS2 + HTTP(S) + BASIC/LDAP based thrift server endpoint This patch provides an option to use HTTP based transport for HiveServer2 endpoint on coordinators that the clients can connect to query. HTTP(S) also works when external TLS is enabled using --ssl_server_certificate. Implemented only for HS2 compatible thrift server since, unlike beeswax, its session management does not need to be tied to the underlying TCP conneciton. Thirft's http transport is modified to support BASIC authentication via ldap. For convenience of developing and reviewing, this patch is based on another that copied THttpServer and THttpTransport into Impala's codebase. TODO = - Fuzz test the http endpoint - Add tests for LDAP + HTTPS Testing === - Parameterized JdbcTest and LdapJdbcTest to work for HS2 + HTTP mode - Added LdapHS2Test, which directly calls into the Hiveserver2 interface using a thrift http client. Manual testing with Beeline client (from Apache Hive), which has builtin support to connect to HTTP(S) based HS2 compatible endpoints. Example -- HTTP mode: > start-impala-cluster.py > JDBC_URL="jdbc:hive2://localhost:/default;transportMode=http" > beeline -u "$JDBC_URL" -- HTTPS mode: > cd $IMPALA_HOME > SSL_ARGS="--ssl_client_ca_certificate=./be/src/testutil/server-cert.pem \ --ssl_server_certificate=./be/src/testutil/server-cert.pem \ --ssl_private_key=./be/src/testutil/server-key.pem --hostname=localhost" > start-impala-cluster.py --impalad_args="$SSL_ARGS" \ --catalogd_args="$SSL_ARGS" --state_store_args="$SSL_ARGS" - Create a local trust store using 'keytool' and import the certificate from server-cert.pem (./clientkeystore in the example). > JDBC_URL="jdbc:hive2://localhost:/default;ssl=true;sslTrustStore= \ ./clientkeystore;trustStorePassword=password;transportMode=http" > beeline -u "$JDBC_URL" -- BASIC Auth with LDAP: > LDAP_ARGS="--enable_ldap_auth --ldap_uri='ldap://...' \ --ldap_bind_pattern='...' --ldap_passwords_in_clear_ok" > start-impala-cluster.py --impalad_args="$LDAP_ARGS" > JDBC_URL="jdbc:hive2://localhost:28000/default;user=...;password=\ ...;transportMode=http" > beeline -u "$JDBC_URL" -- HTTPS mode with LDAP: > start-impala-cluster.py --impalad_args="$LDAP_ARGS $SSL_ARGS" \ --catalogd_args="$SSL_ARGS" --state_store_args="$SSL_ARGS" > JDBC_URL="jdbc:hive2://localhost:28000/default;user=...;password=\ ...;ssl=true;sslTrustStore=./clientkeystore;trustStorePassword=\ password;transportMode=http" > beeline -u "$JDBC_URL" Change-Id: Ic5569ac62ef3af2868b5d0581f5029dac736b2ff --- M be/src/rpc/auth-provider.h M be/src/rpc/authentication-test.cc M be/src/rpc/authentication.cc M be/src/rpc/thrift-server.cc M be/src/rpc/thrift-server.h M be/src/service/impala-server.cc M be/src/service/impala-server.h M be/src/service/impalad-main.cc M be/src/testutil/in-process-servers.cc M be/src/testutil/in-process-servers.h M be/src/transport/THttpServer.cpp M be/src/transport/THttpServer.h M be/src/transport/THttpTransport.cpp M be/src/transport/THttpTransport.h M bin/start-impala-cluster.py M common/thrift/generate_error_codes.py M common/thrift/metrics.json A fe/src/test/java/org/apache/impala/customcluster/LdapHS2Test.java M fe/src/test/java/org/apache/impala/customcluster/LdapJdbcTest.java M fe/src/test/java/org/apache/impala/service/JdbcTest.java M fe/src/test/java/org/apache/impala/service/JdbcTestBase.java M fe/src/test/java/org/apache/impala/testutil/ImpalaJdbcClient.java M fe/src/test/resources/users.ldif M tests/common/impala_cluster.py 24 files changed, 641 insertions(+), 136 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/13299/3 -- To view, visit http://gerrit.cloudera.org:8080/13299 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ic5569ac62ef3af2868b5d0581f5029dac736b2ff Gerrit-Change-Number: 13299 Gerrit-PatchSet: 3 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Mike Yoder Gerrit-Reviewer: Sudhanshu Arora Gerrit-Reviewer: Todd Lipcon
[native-toolchain-CR] Patch Thrift to 0.9.3-p6 to eliminate erroneous ssl warnings.
Thomas Marshall has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13383 ) Change subject: Patch Thrift to 0.9.3-p6 to eliminate erroneous ssl warnings. .. Patch Thrift to 0.9.3-p6 to eliminate erroneous ssl warnings. This will be used to fix "IMPALA-8333: Remove Impala Shell warnings part 2". Remove misleading deprecation warning from the TSSLSocket.validate method. This method is used to determine if the peer's SSL certificate should be validated. The deprecation warning message is a mistake, probably as the result of a confusion of the role of the validate method with the use of a deprecated parameter, also named validate, that is used in the initializer. This code was removed in subsequent Thrift releases. TESTING: I built the toolchain locally and using jenkins. I built impala with the new toolchain and tested that IMPALA-8333 is fixed. Change-Id: I84c2de4e6e3b21a4e3a5591f469d47ab34101687 Reviewed-on: http://gerrit.cloudera.org:8080/13383 Reviewed-by: Thomas Marshall Tested-by: Thomas Marshall --- M buildall.sh A source/thrift/thrift-0.9.3-patches/0006-remove-misleading-deprecation-warning.patch 2 files changed, 29 insertions(+), 0 deletions(-) Approvals: Thomas Marshall: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/13383 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I84c2de4e6e3b21a4e3a5591f469d47ab34101687 Gerrit-Change-Number: 13383 Gerrit-PatchSet: 2 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Thomas Marshall
[native-toolchain-CR] Patch Thrift to 0.9.3-p6 to eliminate erroneous ssl warnings.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13383 ) Change subject: Patch Thrift to 0.9.3-p6 to eliminate erroneous ssl warnings. .. Patch Set 1: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/13383 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84c2de4e6e3b21a4e3a5591f469d47ab34101687 Gerrit-Change-Number: 13383 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Wed, 22 May 2019 16:52:24 + Gerrit-HasComments: No
[native-toolchain-CR] Patch Thrift to 0.9.3-p6 to eliminate erroneous ssl warnings.
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13383 ) Change subject: Patch Thrift to 0.9.3-p6 to eliminate erroneous ssl warnings. .. Patch Set 1: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/13383 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: native-toolchain Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I84c2de4e6e3b21a4e3a5591f469d47ab34101687 Gerrit-Change-Number: 13383 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Sherman Gerrit-Reviewer: Thomas Marshall Gerrit-Comment-Date: Tue, 21 May 2019 18:03:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7290: part 2: Add HS2 support to Impala shell
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12884 ) Change subject: IMPALA-7290: part 2: Add HS2 support to Impala shell .. Patch Set 16: (1 comment) http://gerrit.cloudera.org:8080/#/c/12884/16/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: http://gerrit.cloudera.org:8080/#/c/12884/16/common/thrift/ImpalaService.thrift@545 PS16, Line 545: // Same as HS2 CloseOperation but can return additional information. > I didn't think about that approach but it's an interesting idea. I initiall There's also the issue of running non-Impala clients against Impala, eg. the way we include the hive jdbc driver and use it in our jdbc tests. Also probably not that important of a use case. Are there any plans to update the Impala jdbc driver to use the new API? Definitely agree that encoding this in a string is non-ideal. Also, what does hive do for this? Surely they would also find it useful to return something like num rows modified/errors for dmls? -- To view, visit http://gerrit.cloudera.org:8080/12884 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6d5cc83d545aacc659523f29b1d6feed672e2a12 Gerrit-Change-Number: 12884 Gerrit-PatchSet: 16 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 20 May 2019 20:28:18 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1653: Don't close hiveserver2 session when connection is closed
Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/13306 ) Change subject: IMPALA-1653: Don't close hiveserver2 session when connection is closed .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/13306/4/tests/custom_cluster/test_hs2.py File tests/custom_cluster/test_hs2.py: http://gerrit.cloudera.org:8080/#/c/13306/4/tests/custom_cluster/test_hs2.py@26 PS4, Line 26: > flake8: E302 expected 2 blank lines, found 1 Done -- To view, visit http://gerrit.cloudera.org:8080/13306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78 Gerrit-Change-Number: 13306 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 20 May 2019 19:25:36 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1653: Don't close hiveserver2 session when connection is closed
Hello Michael Ho, Andrew Sherman, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13306 to look at the new patch set (#5). Change subject: IMPALA-1653: Don't close hiveserver2 session when connection is closed .. IMPALA-1653: Don't close hiveserver2 session when connection is closed Currently, when a client connection is closed, we always close any session started over that connection. This is a requirement for beeswax, which always ties sessions to connections, but it is not required for hiveserver2, which allows sessions to be used across connections with a session token. This patch changes this behavior so that hiveserver2 sessions are no longer closed when the corresponding connection is closed. One downside of this change is that clients may inadvertently leave sessions open indefinitely if they close their connection without calling CloseSession(), which can waste space on the coordinator. We already have a flag --idle_session_timeout, but this flag is off by default and sessions that hit this timeout are expired but not fully closed. Rather than changing the default idle session behavior, which could affect existing users, this patch mitigates this issue by adding a new flag: --disconnected_esession_timeout which is set to 1 hour by default. When a session has had no open connections for longer than this time, it will be closed and any associated queries will be unregistered. Testing: - Added e2e tests. Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78 --- M be/src/service/impala-hs2-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h A tests/custom_cluster/test_hs2.py M tests/hs2/test_hs2.py 5 files changed, 230 insertions(+), 56 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/13306/5 -- To view, visit http://gerrit.cloudera.org:8080/13306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78 Gerrit-Change-Number: 13306 Gerrit-PatchSet: 5 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-7290: part 2: Add HS2 support to Impala shell
Thomas Marshall has posted comments on this change. (
http://gerrit.cloudera.org:8080/12884 )
Change subject: IMPALA-7290: part 2: Add HS2 support to Impala shell
..
Patch Set 16:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12884/16/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:
http://gerrit.cloudera.org:8080/#/c/12884/16/be/src/service/client-request-state.h@167
PS16, Line 167: 'request_state'
I assume you mean 'this request'?
http://gerrit.cloudera.org:8080/#/c/12884/16/common/thrift/ImpalaService.thrift
File common/thrift/ImpalaService.thrift:
http://gerrit.cloudera.org:8080/#/c/12884/16/common/thrift/ImpalaService.thrift@453
PS16, Line 453: operation_handle
Unfortunately, our casing in this file is inconsistent, but I think you
probably want this to be 'operationHandle'
http://gerrit.cloudera.org:8080/#/c/12884/16/common/thrift/ImpalaService.thrift@545
PS16, Line 545: // Same as HS2 CloseOperation but can return additional
information.
Did you consider using the SUCCESS_WITH_INFO_STATUS/TStatus::infoMessages to
accomplish the same sort of thing through the regular hs2 interface?
Some reasons I can think of not to do it:
- clients may not be expected it and could have code paths like "if (status !=
SUCCESS_STATUS) then { }'
- It would require flattening the info into a string and then re-parsing on the
other side.
It would be nice to stick closer to vanilla hs2, though.
http://gerrit.cloudera.org:8080/#/c/12884/16/shell/impala_shell.py
File shell/impala_shell.py:
http://gerrit.cloudera.org:8080/#/c/12884/16/shell/impala_shell.py@842
PS16, Line 842: # raise
?
http://gerrit.cloudera.org:8080/#/c/12884/16/tests/hs2/test_hs2.py
File tests/hs2/test_hs2.py:
http://gerrit.cloudera.org:8080/#/c/12884/16/tests/hs2/test_hs2.py@100
PS16, Line 100: # Removed options are returned.
Isn't this still testing that they're not returned?
--
To view, visit http://gerrit.cloudera.org:8080/12884
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d5cc83d545aacc659523f29b1d6feed672e2a12
Gerrit-Change-Number: 12884
Gerrit-PatchSet: 16
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Mon, 20 May 2019 17:44:26 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-1653: Don't close hiveserver2 session when connection is closed
Hello Michael Ho, Andrew Sherman, Tim Armstrong, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/13306 to look at the new patch set (#4). Change subject: IMPALA-1653: Don't close hiveserver2 session when connection is closed .. IMPALA-1653: Don't close hiveserver2 session when connection is closed Currently, when a client connection is closed, we always close any session started over that connection. This is a requirement for beeswax, which always ties sessions to connections, but it is not required for hiveserver2, which allows sessions to be used across connections with a session token. This patch changes this behavior so that hiveserver2 sessions are no longer closed when the corresponding connection is closed. One downside of this change is that clients may inadvertently leave sessions open indefinitely if they close their connection without calling CloseSession(), which can waste space on the coordinator. We already have a flag --idle_session_timeout, but this flag is off by default and sessions that hit this timeout are expired but not fully closed. Rather than changing the default idle session behavior, which could affect existing users, this patch mitigates this issue by adding a new flag: --disconnected_esession_timeout which is set to 1 hour by default. When a session has had no open connections for longer than this time, it will be closed and any associated queries will be unregistered. Testing: - Added e2e tests. Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78 --- M be/src/service/impala-hs2-server.cc M be/src/service/impala-server.cc M be/src/service/impala-server.h A tests/custom_cluster/test_hs2.py M tests/hs2/test_hs2.py 5 files changed, 227 insertions(+), 56 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/06/13306/4 -- To view, visit http://gerrit.cloudera.org:8080/13306 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Ia4555cd9b73db5b4dde92cd4fac4f9bfa3664d78 Gerrit-Change-Number: 13306 Gerrit-PatchSet: 4 Gerrit-Owner: Thomas Marshall Gerrit-Reviewer: Andrew Sherman Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Michael Ho Gerrit-Reviewer: Thomas Marshall Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS integration
Thomas Marshall has posted comments on this change. (
http://gerrit.cloudera.org:8080/13318 )
Change subject: IMPALA-8504: Support CREATE TABLE statement with Kudu/HMS
integration
..
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java
File fe/src/main/java/org/apache/impala/catalog/KuduTable.java:
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@57
PS2, Line 57: private static final Logger LOG =
Logger.getLogger(KuduTable.class);
This isn't used anywhere
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@167
PS2, Line 167: // TODO: validate Kudu is configured to use the same HMS as
Impala.
How difficult would this be to implement and what's the failure mode if its not
the case?
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java
File fe/src/main/java/org/apache/impala/util/KuduUtil.java:
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java@390
PS2, Line 390: String metastoreDbName,
:String
metastoreTableName,
:boolean
isHMSIntegrationEnabled
Please try to match the formatting in the rest of the file, here and elsewhere.
Eg. don't separate all parameters onto their own lines, just wrap as necessary
and indent four spaces.
You might want to look into clang-format-diff as linked here:
https://cwiki.apache.org/confluence/display/IMPALA/Contributing+to+Impala
though note that its more of a guideline than a rule, and in places where it
differs from the existing formatting you should just match what we already do
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/main/java/org/apache/impala/util/KuduUtil.java@397
PS2, Line 397: public static boolean isDefaultKuduTableName(String name,
brief comment
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/KuduHMSIntegrationTest.java
File fe/src/test/java/org/apache/impala/analysis/KuduHMSIntegrationTest.java:
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/KuduHMSIntegrationTest.java@39
PS2, Line 39: public class KuduHMSIntegrationTest {
This makes me nervous. We don't currently have any tests that restart any
minicluster components like this. There are a lot of tests that will run after
this that depend on the minicluster being set up in a particular way and we
should be careful not to mess with those.
I think that what we should do is essentially define a new test suite that will
run at the very end.
I have a patch out right now that does something similar:
https://gerrit.cloudera.org/#/c/13337/ You can copy what I've done there:
- Put this class in its own package, say: org.apache.impala.customservice
- Copy the changes I made to bin/run-all-tests.sh with appropriate modifications
- It would also be great if you could define a generic
'restartMiniclusterComponent' function in a utility class like the
CustomClusterRunner class that I added that takes a component name (i.e.
'kudu') and a set of extra env variables to set and then use that in
setup/cleanup
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java:
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@24
PS2, Line 24: import org.apache.impala.catalog.KuduTable;
nit: not needed
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java:
http://gerrit.cloudera.org:8080/#/c/13318/2/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@51
PS2, Line 51: import org.apache.impala.catalog.KuduTable;
nit: not needed
--
To view, visit http://gerrit.cloudera.org:8080/13318
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I465673d749221bd5f3772814b1c22c2673a53f5c
Gerrit-Change-Number: 13318
Gerrit-PatchSet: 2
Gerrit-Owner: Hao Hao
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Hao Hao
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Thomas Marshall
Gerrit-Comment-Date: Fri, 17 May 2019 21:38:32 +
Gerrit-HasComments: Yes
