kyuubi-beeline auto constructs JDBC URL from kyuubi-defaults.conf #6339
Conversation
| try { | ||
| Map<String, String> propsMap = getPropertiesFromFile(propsFile); | ||
| StringBuilder hiveConf = new StringBuilder(); | ||
| for (Entry<String, String> kv : propsMap.entrySet()) { |
There was a problem hiding this comment.
Load all configurations? Should we use a fixed prefix like BEELINE_CONNECTION_PROPERTY_PREFIX?
There was a problem hiding this comment.
the intention is infer Kyuubi connection info from the existing kyuubi-defaults.conf, instead supporting configuring beeline specific configurations inside kyuubi-defaults.conf
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| // get the connection properties from user specific config file | ||
| Properties properties = kyuubiConfFileParser.getConnectionProperties(); | ||
| for (String key : properties.stringPropertyNames()) { | ||
| userConnectionProperties.setProperty(key, properties.getProperty(key)); |
There was a problem hiding this comment.
We should merge HIVE_CONF_PROPERTY_KEY instead of overwriting.
|
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6339 +/- ##
============================================
- Coverage 58.50% 58.41% -0.09%
Complexity 24 24
============================================
Files 653 655 +2
Lines 39775 40052 +277
Branches 5473 5505 +32
============================================
+ Hits 23269 23397 +128
- Misses 14022 14146 +124
- Partials 2484 2509 +25 ☔ View full report in Codecov by Sentry. |
🔍 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 🔖
Test Plan 🧪
Behavior Without This Pull Request ⚰️
Behavior With This Pull Request 🎉
Related Unit Tests
Checklist 📝
Be nice. Be informative.