From 1d4fd5c6eacab0b88f8660f9d780174434393f1a Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Sun, 13 Nov 2022 18:22:17 +0100 Subject: [PATCH] The pinot-admin.sh command is now hard-coded. (#27641) --- airflow/providers/apache/pinot/CHANGELOG.rst | 22 +++++++++++++++++++ airflow/providers/apache/pinot/hooks/pinot.py | 16 +++++++++++--- airflow/providers/apache/pinot/provider.yaml | 1 + .../apache/pinot/hooks/test_pinot.py | 17 ++++++++++---- 4 files changed, 49 insertions(+), 7 deletions(-) diff --git a/airflow/providers/apache/pinot/CHANGELOG.rst b/airflow/providers/apache/pinot/CHANGELOG.rst index 9c6c8dbf106ce..6eb2608d76261 100644 --- a/airflow/providers/apache/pinot/CHANGELOG.rst +++ b/airflow/providers/apache/pinot/CHANGELOG.rst @@ -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 ..... diff --git a/airflow/providers/apache/pinot/hooks/pinot.py b/airflow/providers/apache/pinot/hooks/pinot.py index 263568d6a0c9f..909b0182ddcb5 100644 --- a/airflow/providers/apache/pinot/hooks/pinot.py +++ b/airflow/providers/apache/pinot/hooks/pinot.py @@ -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. @@ -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 ) @@ -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: diff --git a/airflow/providers/apache/pinot/provider.yaml b/airflow/providers/apache/pinot/provider.yaml index d4f0c266b556c..9877783a6881d 100644 --- a/airflow/providers/apache/pinot/provider.yaml +++ b/airflow/providers/apache/pinot/provider.yaml @@ -22,6 +22,7 @@ description: | `Apache Pinot `__ versions: + - 4.0.0 - 3.2.1 - 3.2.0 - 3.1.0 diff --git a/tests/providers/apache/pinot/hooks/test_pinot.py b/tests/providers/apache/pinot/hooks/test_pinot.py index de79691633b67..a9b25a0480f17 100644 --- a/tests/providers/apache/pinot/hooks/test_pinot.py +++ b/tests/providers/apache/pinot/hooks/test_pinot.py @@ -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): @@ -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 ) @@ -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 ) @@ -196,7 +196,7 @@ 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( @@ -204,6 +204,15 @@ def test_run_cli_failure_status_code(self, mock_popen): ) +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()