New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
kyuubi-beeline auto constructs JDBC URL from kyuubi-defaults.conf #6339
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Load all configurations? Should we use a fixed prefix like BEELINE_CONNECTION_PROPERTY_PREFIX
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should merge HIVE_CONF_PROPERTY_KEY
instead of overwriting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
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.