Skip to content

Commit

Permalink
The pinot-admin.sh command is now hard-coded.
Browse files Browse the repository at this point in the history
  • Loading branch information
potiuk committed Nov 13, 2022
1 parent 52593b0 commit ec5eb42
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 7 deletions.
22 changes: 22 additions & 0 deletions airflow/providers/apache/pinot/CHANGELOG.rst
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
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
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
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

0 comments on commit ec5eb42

Please sign in to comment.