PHOENIX-7848. Use ZK TLS properties from HBase config if present#2467
PHOENIX-7848. Use ZK TLS properties from HBase config if present#2467anmolnar wants to merge 2 commits into
Conversation
| '-Dzookeeper.ssl.trustStore.location=' + zkcfg['ssl.trustStore.location'] + ' ' + \ | ||
| '-Dzookeeper.ssl.trustStore.type=' + zkcfg['ssl.trustStore.type'] + ' ' + \ | ||
| '-Dzookeeper.ssl.trustStore.password=' + zkcfg['ssl.trustStore.password'] + ' ' | ||
|
|
There was a problem hiding this comment.
Can you please add Exception handling to this?
We want to continue if there is some problem with parsing the zookeeper parameters.
Please add a default "" value to zk_tls_args in case we don't have hbase.zookeeper.property.client.secure property.
There was a problem hiding this comment.
Default "" doesn't make any difference for this field. The field will be undefined. We have two options: use the getattr call as below, or define zk_tls_args at top level of this file.
Certainly I can add exception handler, but there's no exception handling anywhere else in this file. Does it make sense?
There was a problem hiding this comment.
I think so, yes
There are much more thinks that could go wrong here.
There was a problem hiding this comment.
I prefer the later option, but I other variables are not defined at top level of the file, just with global
There was a problem hiding this comment.
There could be better error when some of the keys are missing
|
|
||
| java_cmd = phoenix_utils.java + ' ' + phoenix_utils.jvm_module_flags + \ | ||
| ' ' + opts + \ | ||
| ' ' + getattr(phoenix_utils, "zk_tls_args", "") + \ |
There was a problem hiding this comment.
If we make sure zk_tls_args has some value, we could have phoenix_utils.zk_tls_args here
There was a problem hiding this comment.
No, we cannot. If it doesn't get any value in the utils, it will be undefined and this code would throw an exception (tested).
|
@richardantal I moved I'm not sure about the exception handler. What should be the logic inside the exception handler? I don't want the error to be swallowed and ignored, because if there's anything wrong with the configuration, the tool should refuse to work. |
| for prop in root.findall("property") | ||
| if prop.find("name").text.startswith(zk_hbase_prefix) | ||
| } | ||
| if zkcfg.get('client.secure').lower() == 'true': |
There was a problem hiding this comment.
We should check if 'client.secure' in zkcfg too
2 participants
What changes were proposed in this pull request?
Parse ZooKeeper TLS properties from
hbase-site.xmland add them tosqllinecommand line args.Why are the changes needed?
Phoenix
sqllinethick client is unable to connect to TLS-only ZooKeeper ensemble.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Locally.
Was this patch authored or co-authored using generative AI tooling?
No.