You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "amoghrajesh (via GitHub)" <gi...@apache.org> on 2024/04/01 08:45:13 UTC
[PR] Adding support to hive hook for high availability Hive installations [airflow]
amoghrajesh opened a new pull request, #38651:
URL: https://github.com/apache/airflow/pull/38651
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
Right now there is a limitation where the HiveOperator incorrectly parses and constructs the beeline command if hive "high availability" url is given for host. I was able to work this around by adding this change to extra and changing the HiveHook code but a better fix would be to add a new field called HA, which when selected, parses the connection better in the hook itself.
I would like to propose adding this into the connection form for Hive CLI type in a better way. Recently some change was made to remove extra and integrate this into the form and simplify it: https://github.com/apache/airflow/pull/37043/.
Would propose adding similar fields for HA and if enabled, the beeline command construction would vary slightly.
MY HA URL looks somewhat like this btw: `jdbc:hive2://host1:port1,host2:port2,host3:port3/default;principal=hive/principal@REALM;serviceDiscoveryMode=zooKeeper;ssl=true;zooKeeperNamespace=hiveserver2`
Tested using a HA setting, beeline works as expected.
<!-- Please keep an empty line above the dashes. -->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding support to hive hook for high availability Hive installations [airflow]
Posted by "romsharon98 (via GitHub)" <gi...@apache.org>.
romsharon98 commented on PR #38651:
URL: https://github.com/apache/airflow/pull/38651#issuecomment-2029518394
Looks good :)
Can u just add test with `proxy_user` in `extra_json` just to verify that it add `;` correctly and to verify someone else not remove it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding support to hive hook for high availability Hive installations [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #38651:
URL: https://github.com/apache/airflow/pull/38651#issuecomment-2029434704
This is how the form looks now:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding support to hive hook for high availability Hive installations [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #38651:
URL: https://github.com/apache/airflow/pull/38651#discussion_r1546303780
##########
airflow/providers/apache/hive/hooks/hive.py:
##########
@@ -100,6 +100,9 @@ def __init__(
) -> None:
super().__init__()
conn = self.get_connection(hive_cli_conn_id)
+
+ print("conn is from __init__", conn)
Review Comment:
Do we need this print?
This is going to be printed in the logs everything init is invoked when parsing the dag
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding support to hive hook for high availability Hive installations [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh merged PR #38651:
URL: https://github.com/apache/airflow/pull/38651
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding support to hive hook for high availability Hive installations [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #38651:
URL: https://github.com/apache/airflow/pull/38651#discussion_r1549113214
##########
airflow/providers/apache/hive/hooks/hive.py:
##########
@@ -160,6 +162,9 @@ def _prepare_cli_cmd(self) -> list[Any]:
hive_bin = "beeline"
self._validate_beeline_parameters(conn)
jdbc_url = f"jdbc:hive2://{conn.host}:{conn.port}/{conn.schema}"
Review Comment:
Better to put this in the else block
##########
airflow/providers/apache/hive/hooks/hive.py:
##########
@@ -170,6 +175,10 @@ def _prepare_cli_cmd(self) -> list[Any]:
if ";" in proxy_user:
raise RuntimeError("The proxy_user should not contain the ';' character")
jdbc_url += f";principal={template};{proxy_user}"
+ if self.high_availability:
+ if proxy_user:
+ jdbc_url += ";"
Review Comment:
You can check if the jdbc_url ends with ;
##########
airflow/providers/apache/hive/hooks/hive.py:
##########
@@ -100,6 +100,9 @@ def __init__(
) -> None:
super().__init__()
conn = self.get_connection(hive_cli_conn_id)
+
+ print("conn is from __init__", conn)
+
Review Comment:
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding support to hive hook for high availability Hive installations [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #38651:
URL: https://github.com/apache/airflow/pull/38651#discussion_r1549306722
##########
airflow/providers/apache/hive/hooks/hive.py:
##########
@@ -160,6 +162,9 @@ def _prepare_cli_cmd(self) -> list[Any]:
hive_bin = "beeline"
self._validate_beeline_parameters(conn)
jdbc_url = f"jdbc:hive2://{conn.host}:{conn.port}/{conn.schema}"
Review Comment:
Sure, making the changes
##########
airflow/providers/apache/hive/hooks/hive.py:
##########
@@ -170,6 +175,10 @@ def _prepare_cli_cmd(self) -> list[Any]:
if ";" in proxy_user:
raise RuntimeError("The proxy_user should not contain the ';' character")
jdbc_url += f";principal={template};{proxy_user}"
+ if self.high_availability:
+ if proxy_user:
+ jdbc_url += ";"
Review Comment:
Yeah makes sense. Let me add that instead of this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding support to hive hook for high availability Hive installations [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #38651:
URL: https://github.com/apache/airflow/pull/38651#discussion_r1547114486
##########
airflow/providers/apache/hive/hooks/hive.py:
##########
@@ -100,6 +100,9 @@ def __init__(
) -> None:
super().__init__()
conn = self.get_connection(hive_cli_conn_id)
+
+ print("conn is from __init__", conn)
Review Comment:
No, my bad, had added during debugging, forgot to remove it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding support to hive hook for high availability Hive installations [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #38651:
URL: https://github.com/apache/airflow/pull/38651#discussion_r1548055690
##########
airflow/providers/apache/hive/hooks/hive.py:
##########
@@ -186,7 +195,13 @@ def _prepare_cli_cmd(self) -> list[Any]:
return [hive_bin, *cmd_extra, *hive_params_list]
def _validate_beeline_parameters(self, conn):
- if ":" in conn.host or "/" in conn.host or ";" in conn.host:
+ if self.high_availability:
Review Comment:
The reason this was done was for a refactor from earlier. The `schema` still cannot contain `;`
Earlier it was like this:
```
def _validate_beeline_parameters(self, conn):
if ":" in conn.host or "/" in conn.host or ";" in conn.host:
raise Exception(
f"The host used in beeline command ({conn.host}) should not contain ':/;' characters)"
)
try:
int_port = int(conn.port)
if not 0 < int_port <= 65535:
raise Exception(f"The port used in beeline command ({conn.port}) should be in range 0-65535)")
except (ValueError, TypeError) as e:
raise Exception(f"The port used in beeline command ({conn.port}) should be a valid integer: {e})")
if ";" in conn.schema:
raise Exception(
f"The schema used in beeline command ({conn.schema}) should not contain ';' character)"
)
```
To accommodate this logic:
1. Do not validate port when HA is given, because host will be of form: `host1:port1, host2:port2...`
2. We need to validate condition where `;` should not be present even for HA connections.
So basically it became:
```python
def _validate_beeline_parameters(self, conn):
**if self.high_availability:
if ";" in conn.schema:
raise Exception(
f"The schema used in beeline command ({conn.schema}) should not contain ';' character)"
)
return
elif** ":" in conn.host or "/" in conn.host or ";" in conn.host:
raise Exception(
f"The host used in beeline command ({conn.host}) should not contain ':/;' characters)"
)
try:
int_port = int(conn.port)
if not 0 < int_port <= 65535:
raise Exception(f"The port used in beeline command ({conn.port}) should be in range 0-65535)")
except (ValueError, TypeError) as e:
raise Exception(f"The port used in beeline command ({conn.port}) should be a valid integer: {e})")
if ";" in conn.schema:
raise Exception(
f"The schema used in beeline command ({conn.schema}) should not contain ';' character)"
)
```
(Highlighted code is the delta)
@potiuk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding support to hive hook for high availability Hive installations [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #38651:
URL: https://github.com/apache/airflow/pull/38651#issuecomment-2029586589
> Looks good :) Can u just add test with `proxy_user` in `extra_json` just to verify that it add `;` correctly and to verify someone else not remove it.
Good catch! Updated the test, can you check once?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding support to hive hook for high availability Hive installations [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #38651:
URL: https://github.com/apache/airflow/pull/38651#issuecomment-2034004355
@hussein-awala I have addressed the nits as well, can you take a look if it looks OK now?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding support to hive hook for high availability Hive installations [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #38651:
URL: https://github.com/apache/airflow/pull/38651#issuecomment-2033657749
@potiuk @eladkal could you review this when you have some time? Or who would be the right person for hive related PRs?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding support to hive hook for high availability Hive installations [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #38651:
URL: https://github.com/apache/airflow/pull/38651#discussion_r1548055690
##########
airflow/providers/apache/hive/hooks/hive.py:
##########
@@ -186,7 +195,13 @@ def _prepare_cli_cmd(self) -> list[Any]:
return [hive_bin, *cmd_extra, *hive_params_list]
def _validate_beeline_parameters(self, conn):
- if ":" in conn.host or "/" in conn.host or ";" in conn.host:
+ if self.high_availability:
Review Comment:
The reason this was done was for a refactor from earlier. The `schema` still cannot contain `;`
Earlier it was like this:
```
def _validate_beeline_parameters(self, conn):
if ":" in conn.host or "/" in conn.host or ";" in conn.host:
raise Exception(
f"The host used in beeline command ({conn.host}) should not contain ':/;' characters)"
)
try:
int_port = int(conn.port)
if not 0 < int_port <= 65535:
raise Exception(f"The port used in beeline command ({conn.port}) should be in range 0-65535)")
except (ValueError, TypeError) as e:
raise Exception(f"The port used in beeline command ({conn.port}) should be a valid integer: {e})")
if ";" in conn.schema:
raise Exception(
f"The schema used in beeline command ({conn.schema}) should not contain ';' character)"
)
```
To accommodate this logic:
1. Do not validate port when HA is given, because host will be of form: `host1:port1, host2:port2...`
2. We need to validate condition where `;` should not be present even for HA connections.
So basically it became:
def _validate_beeline_parameters(self, conn):
**if self.high_availability:
if ";" in conn.schema:
raise Exception(
f"The schema used in beeline command ({conn.schema}) should not contain ';' character)"
)
return
elif** ":" in conn.host or "/" in conn.host or ";" in conn.host:
raise Exception(
f"The host used in beeline command ({conn.host}) should not contain ':/;' characters)"
)
try:
int_port = int(conn.port)
if not 0 < int_port <= 65535:
raise Exception(f"The port used in beeline command ({conn.port}) should be in range 0-65535)")
except (ValueError, TypeError) as e:
raise Exception(f"The port used in beeline command ({conn.port}) should be a valid integer: {e})")
if ";" in conn.schema:
raise Exception(
f"The schema used in beeline command ({conn.schema}) should not contain ';' character)"
)
(Highlighted code is the delta)
@potiuk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding support to hive hook for high availability Hive installations [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #38651:
URL: https://github.com/apache/airflow/pull/38651#discussion_r1547805091
##########
airflow/providers/apache/hive/hooks/hive.py:
##########
@@ -186,7 +195,13 @@ def _prepare_cli_cmd(self) -> list[Any]:
return [hive_bin, *cmd_extra, *hive_params_list]
def _validate_beeline_parameters(self, conn):
- if ":" in conn.host or "/" in conn.host or ";" in conn.host:
+ if self.high_availability:
Review Comment:
This one is somewhat of a security feature - is this expected that `schema` for HA can contain `:` or `/` ? Some examples? I believe all the zookeper etc. parameters will be added anyway above, so there is no particular need to have `/` or `:` in the schema?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding support to hive hook for high availability Hive installations [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #38651:
URL: https://github.com/apache/airflow/pull/38651#discussion_r1546123813
##########
airflow/providers/apache/hive/hooks/hive.py:
##########
@@ -170,6 +178,10 @@ def _prepare_cli_cmd(self) -> list[Any]:
if ";" in proxy_user:
raise RuntimeError("The proxy_user should not contain the ';' character")
jdbc_url += f";principal={template};{proxy_user}"
+ if self.high_availability:
+ if proxy_user:
+ jdbc_url += ";"
+ jdbc_url += "serviceDiscoveryMode=zooKeeper;ssl=true;zooKeeperNamespace=hiveserver2"
Review Comment:
Fields needed in presence of HA, usually zookeeper will be installed in HA hive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding support to hive hook for high availability Hive installations [airflow]
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on code in PR #38651:
URL: https://github.com/apache/airflow/pull/38651#discussion_r1549126122
##########
airflow/providers/apache/hive/hooks/hive.py:
##########
@@ -100,6 +100,9 @@ def __init__(
) -> None:
super().__init__()
conn = self.get_connection(hive_cli_conn_id)
+
+ print("conn is from __init__", conn)
+
Review Comment:
This is already resolved
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding support to hive hook for high availability Hive installations [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #38651:
URL: https://github.com/apache/airflow/pull/38651#issuecomment-2036692513
@potiuk @eladkal @hussein-awala if all looks good shall we send this one in?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Adding support to hive hook for high availability Hive installations [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #38651:
URL: https://github.com/apache/airflow/pull/38651#issuecomment-2037396993
Merging this in favour of the reviews from Hussein and Elad.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org