Skip to content
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

The pinot-admin.sh command is now hard-coded. #27641

Merged
merged 1 commit into from
Nov 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 22 additions & 0 deletions airflow/providers/apache/pinot/CHANGELOG.rst
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,28 @@
Changelog
---------

4.0.0
.....

This release of provider is only available for Airflow 2.3+ as explained in the Apache Airflow
providers support policy https://github.com/apache/airflow/blob/main/README.md#support-for-providers

Breaking changes
~~~~~~~~~~~~~~~~

The admin command is now hard-coded to ``pinot-admin.sh``. The ``pinot-admin.sh`` command must be available
on the path in order to use PinotAdminHook.

Misc
~~~~

* ``Move min airflow version to 2.3.0 for all providers (#27196)``
* ``Bump pinotdb version (#27201)``

.. Below changes are excluded from the changelog. Move them to
appropriate section above if needed. Do not delete the lines(!):
* ``Enable string normalization in python formatting - providers (#27205)``

3.2.1
.....

Expand Down
16 changes: 13 additions & 3 deletions airflow/providers/apache/pinot/hooks/pinot.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ class PinotAdminHook(BaseHook):
following PR: https://github.com/apache/incubator-pinot/pull/4110

:param conn_id: The name of the connection to use.
:param cmd_path: The filepath to the pinot-admin.sh executable
:param cmd_path: Do not modify the parameter. It used to be the filepath to the pinot-admin.sh
executable but in version 4.0.0 of apache-pinot provider, value of this parameter must
remain the default value: `pinot-admin.sh`. It is left here to not accidentally override
the `pinot_admin_system_exit` in case positional parameters were used to initialize the hook.
:param pinot_admin_system_exit: If true, the result is evaluated based on the status code.
Otherwise, the result is evaluated as a failure if "Error" or
"Exception" is in the output message.
Expand All @@ -62,7 +65,15 @@ def __init__(
conn = self.get_connection(conn_id)
self.host = conn.host
self.port = str(conn.port)
self.cmd_path = conn.extra_dejson.get("cmd_path", cmd_path)
if cmd_path != "pinot-admin.sh":
raise RuntimeError(
"In version 4.0.0 of the PinotAdminHook the cmd_path has been hard-coded to"
" pinot-admin.sh. In order to avoid accidental using of this parameter as"
" positional `pinot_admin_system_exit` the `cmd_parameter`"
" parameter is left here but you should not modify it. Make sure that "
" `pinot-admin.sh` is on your PATH and do not change cmd_path value."
)
self.cmd_path = "pinot-admin.sh"
self.pinot_admin_system_exit = conn.extra_dejson.get(
"pinot_admin_system_exit", pinot_admin_system_exit
)
Expand Down Expand Up @@ -213,7 +224,6 @@ def run_cli(self, cmd: list[str], verbose: bool = True) -> str:

if verbose:
self.log.info(" ".join(command))

with subprocess.Popen(
command, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, close_fds=True, env=env
) as sub_process:
Expand Down
1 change: 1 addition & 0 deletions airflow/providers/apache/pinot/provider.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ description: |
`Apache Pinot <https://pinot.apache.org/>`__

versions:
- 4.0.0
- 3.2.1
- 3.2.0
- 3.1.0
Expand Down
17 changes: 13 additions & 4 deletions tests/providers/apache/pinot/hooks/test_pinot.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def setUp(self):
self.conn = conn = mock.MagicMock()
self.conn.host = "host"
self.conn.port = "1000"
self.conn.extra_dejson = {"cmd_path": "./pinot-admin.sh"}
self.conn.extra_dejson = {}

class PinotAdminHookTest(PinotAdminHook):
def get_connection(self, conn_id):
Expand Down Expand Up @@ -165,7 +165,7 @@ def test_run_cli_success(self, mock_popen):

params = ["foo", "bar", "baz"]
self.db_hook.run_cli(params)
params.insert(0, self.conn.extra_dejson.get("cmd_path"))
params.insert(0, "pinot-admin.sh")
mock_popen.assert_called_once_with(
params, stderr=subprocess.STDOUT, stdout=subprocess.PIPE, close_fds=True, env=None
)
Expand All @@ -180,7 +180,7 @@ def test_run_cli_failure_error_message(self, mock_popen):
params = ["foo", "bar", "baz"]
with pytest.raises(AirflowException):
self.db_hook.run_cli(params)
params.insert(0, self.conn.extra_dejson.get("cmd_path"))
params.insert(0, "pinot-admin.sh")
mock_popen.assert_called_once_with(
params, stderr=subprocess.STDOUT, stdout=subprocess.PIPE, close_fds=True, env=None
)
Expand All @@ -196,14 +196,23 @@ def test_run_cli_failure_status_code(self, mock_popen):
params = ["foo", "bar", "baz"]
with pytest.raises(AirflowException):
self.db_hook.run_cli(params)
params.insert(0, self.conn.extra_dejson.get("cmd_path"))
params.insert(0, "pinot-admin.sh")
env = os.environ.copy()
env.update({"JAVA_OPTS": "-Dpinot.admin.system.exit=true "})
mock_popen.assert_called_once_with(
params, stderr=subprocess.STDOUT, stdout=subprocess.PIPE, close_fds=True, env=env
)


class TestPinotAdminHookCreation:
def test_exception_when_overriding_cmd_path(self):
with pytest.raises(RuntimeError):
PinotAdminHook(cmd_path="some_path.sh")

def test_exception_when_keeping_cmd_path(self):
PinotAdminHook(cmd_path="pinot-admin.sh")


class TestPinotDbApiHook(unittest.TestCase):
def setUp(self):
super().setUp()
Expand Down