kyuubi-beeline auto constructs JDBC URL from kyuubi-defaults.conf #6339

zhaohehuhu · 2024-04-26T07:45:08Z

🔍 Description

Issue References 🔗

as title
This pull request closes #6255

Describe Your Solution 🔧

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Types of changes 🔖

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklist 📝

This patch was not authored or co-authored using Generative Tooling

Be nice. Be informative.

wForget · 2024-04-26T08:35:42Z

...i-hive-beeline/src/main/java/org/apache/hive/beeline/hs2connection/KyuubiConfFileParser.java

+ try {
+ Map<String, String> propsMap = getPropertiesFromFile(propsFile);
+ StringBuilder hiveConf = new StringBuilder();
+ for (Entry<String, String> kv : propsMap.entrySet()) {


Load all configurations? Should we use a fixed prefix like BEELINE_CONNECTION_PROPERTY_PREFIX?

the intention is infer Kyuubi connection info from the existing kyuubi-defaults.conf, instead supporting configuring beeline specific configurations inside kyuubi-defaults.conf

Currently, most of configurations in kyuubi-defaults.conf are server-side configurations. We can define a prefix to determine whether it is a client config, and then add them to hive_conf of jdbc url.

In addition to inferring HIVE_CONF_PROPERTY_KEY, we also need to infer HOST_PROPERTY_KEY based on server configuration.

We can define a prefix to determine whether it is a client config

beeline-site.xml takes responsibility for that, I don't think we should propagate it to kyuubi-defaults.conf

Why do we need HOST_PROPERTY_KEY?

beeline-site.xml takes responsibility for that, I don't think we should propagate it to kyuubi-defaults.conf

In addition to kyuubi server host and port, what other valid configurations can we infer in the current kyuubi-defaults.conf?

Why do we need HOST_PROPERTY_KEY?

Pass server host and port configured in kyuubi-defaults.conf to jdbc url.

wForget · 2024-04-26T08:36:48Z

kyuubi-hive-beeline/src/main/java/org/apache/hive/beeline/BeeLine.java

+ // get the connection properties from user specific config file
+ Properties properties = kyuubiConfFileParser.getConnectionProperties();
+ for (String key : properties.stringPropertyNames()) {
+ userConnectionProperties.setProperty(key, properties.getProperty(key));


We should merge HIVE_CONF_PROPERTY_KEY instead of overwriting.

pan3793 · 2024-04-26T09:11:55Z

we need the design before implementation.

what's the priority of those configuration files? does our change break the compatibility of vanilla Hive BeeLine? should we merge configurations from different files?

codecov-commenter · 2024-05-10T08:42:25Z

Codecov Report

Attention: Patch coverage is 0% with 150 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (f7e8d69) to head (c90265e).
Report is 1 commits behind head on master.

Files	Patch %	Lines
...ve/beeline/hs2connection/KyuubiConfFileParser.java	0.00%	87 Missing ⚠️
...rc/main/java/org/apache/kyuubi/util/JavaUtils.java	0.00%	30 Missing ⚠️
...src/main/java/org/apache/hive/beeline/BeeLine.java	0.00%	12 Missing ⚠️
...ne/hs2connection/KyuubiConfFileParseException.java	0.00%	4 Missing ⚠️
...ine/hs2connection/UserHS2ConnectionFileParser.java	0.00%	4 Missing ⚠️
...ache/kyuubi/server/KyuubiRestFrontendService.scala	0.00%	3 Missing ⚠️
...ache/kyuubi/engine/spark/SparkProcessBuilder.scala	0.00%	2 Missing ⚠️
...che/kyuubi/server/KyuubiTrinoFrontendService.scala	0.00%	2 Missing ⚠️
...rg/apache/kyuubi/zookeeper/EmbeddedZookeeper.scala	0.00%	2 Missing ⚠️
...rg/apache/kyuubi/engine/spark/SparkSQLEngine.scala	0.00%	1 Missing ⚠️
... and 3 more

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #6339    +/-   ##
=======================================
  Coverage    0.00%   0.00%            
=======================================
  Files         675     677     +2     
  Lines       41641   41744   +103     
  Branches     5685    5702    +17     
=======================================
- Misses      41641   41744   +103

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zhaohehuhu · 2024-06-04T02:30:23Z

plz help review again @pan3793 @wForget. Thanks!

pan3793 · 2024-06-04T02:32:11Z

thanks for update, will take a look soon

zhaohehuhu · 2024-06-04T03:08:01Z

thanks for update, will take a look soon

Appreciate it.

pan3793 · 2024-06-04T13:51:11Z

docs/client/cli/kyuubi_beeline.md

+
+2. `beeline-hs2-connection.xml`: This file is checked next, and any properties in this file take precedence over those in `beeline-site.xml`.
+
+3. `kyuubi-defaults.conf`: Properties from this file are loaded next. Values in this file may merge with the values of hiveconf from `beeline-hs2-connection.xml`.


the "merge" behavior is confusing here, which one will take effect for duplicated properties?

They are both appended to the URL, but the latter one will take effect in case of duplicated properties.

pan3793 · 2024-06-04T13:52:12Z

@zhaohehuhu before looking into the code, I read and modified kyuubi_beeline.md, almost good, there is still one question on the docs, could you please clarify it?

pan3793 · 2024-06-04T13:52:53Z

Also cc @wForget, do you think the behavior described in the docs make sense?

pan3793 · 2024-06-04T13:54:24Z

docs/client/cli/kyuubi_beeline.md

+| kyuubi.authentication.ldap.userFilter | comma-separated values are not supported |
+| kyuubi.batch.conf.ignore.list | comma-separated values are not supported |
+| kyuubi.frontend.protocols | comma-separated values are not supported |
+| kyuubi.frontend.thrift.binary.ssl.disallowed.protocols | The value(REST) is an existing keyword in Beeline |


let's use word "Kyuubi Beeline" to distinguish with Hive Beeline

Suggested change

| kyuubi.frontend.thrift.binary.ssl.disallowed.protocols | The value(REST) is an existing keyword in Beeline |

| kyuubi.frontend.thrift.binary.ssl.disallowed.protocols | The value(REST) is an existing keyword in Kyuubi Beeline |

Got it. Thanks.

docs/client/cli/kyuubi_beeline.md

github-actions bot added module:hive module:ctl labels Apr 26, 2024

pan3793 requested a review from wForget April 26, 2024 08:01

wForget reviewed Apr 26, 2024

View reviewed changes

github-actions bot added the kind:documentation Documentation is a feature! label May 10, 2024

zhaohehuhu requested review from wForget and pan3793 June 4, 2024 02:30

pan3793 reviewed Jun 4, 2024

View reviewed changes

docs/client/cli/kyuubi_beeline.md Outdated Show resolved Hide resolved

rewrite

74fa1c4

pan3793 force-pushed the dev-0426 branch from 48689a3 to 74fa1c4 Compare June 11, 2024 10:40

github-actions bot added module:server module:spark module:common kind:build module:ha labels Jun 11, 2024

pan3793 added 5 commits June 11, 2024 19:14

fix import

d84ff27

utils

3033d8f

fix import

aa31bcf

fix

c90265e

fix

6e2ab65

github-actions bot added the module:kubernetes label Jun 11, 2024

github-actions bot added the module:integration-tests label Jun 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kyuubi-beeline auto constructs JDBC URL from kyuubi-defaults.conf #6339

kyuubi-beeline auto constructs JDBC URL from kyuubi-defaults.conf #6339

zhaohehuhu commented Apr 26, 2024 •

edited

wForget Apr 26, 2024

pan3793 Apr 26, 2024

wForget Apr 26, 2024

pan3793 Apr 26, 2024

wForget Apr 26, 2024

wForget Apr 26, 2024

zhaohehuhu May 10, 2024

pan3793 commented Apr 26, 2024 •

edited

codecov-commenter commented May 10, 2024 •

edited

zhaohehuhu commented Jun 4, 2024

pan3793 commented Jun 4, 2024

zhaohehuhu commented Jun 4, 2024

pan3793 Jun 4, 2024

zhaohehuhu Jun 5, 2024

pan3793 commented Jun 4, 2024

pan3793 commented Jun 4, 2024

pan3793 Jun 4, 2024

zhaohehuhu Jun 5, 2024


		2. `beeline-hs2-connection.xml`: This file is checked next, and any properties in this file take precedence over those in `beeline-site.xml`.

		3. `kyuubi-defaults.conf`: Properties from this file are loaded next. Values in this file may merge with the values of hiveconf from `beeline-hs2-connection.xml`.

	\| kyuubi.frontend.thrift.binary.ssl.disallowed.protocols \| The value(REST) is an existing keyword in Beeline \|
	\| kyuubi.frontend.thrift.binary.ssl.disallowed.protocols \| The value(REST) is an existing keyword in Kyuubi Beeline \|

kyuubi-beeline auto constructs JDBC URL from kyuubi-defaults.conf #6339

Are you sure you want to change the base?

kyuubi-beeline auto constructs JDBC URL from kyuubi-defaults.conf #6339

Conversation

zhaohehuhu commented Apr 26, 2024 • edited

🔍 Description

Issue References 🔗

Describe Your Solution 🔧

Types of changes 🔖

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklist 📝

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pan3793 commented Apr 26, 2024 • edited

codecov-commenter commented May 10, 2024 • edited

Codecov Report

zhaohehuhu commented Jun 4, 2024

pan3793 commented Jun 4, 2024

zhaohehuhu commented Jun 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pan3793 commented Jun 4, 2024

pan3793 commented Jun 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhaohehuhu commented Apr 26, 2024 •

edited

pan3793 commented Apr 26, 2024 •

edited

codecov-commenter commented May 10, 2024 •

edited