jens-scheffler-bosch commented on code in PR #32113:
URL: https://github.com/apache/airflow/pull/32113#discussion_r1245733099
##########
airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -562,11 +577,38 @@ def extract_xcom(self, pod: V1Pod) -> str:
resp,
f"if [ -s {PodDefaults.XCOM_MOUNT_PATH}/return.json ]; then
cat {PodDefaults.XCOM_MOUNT_PATH}/return.json; else echo
__airflow_xcom_result_empty__; fi", # noqa
)
- self._exec_pod_command(resp, "kill -s SIGINT 1")
+ if isinstance(result, str) and result.rstrip() !=
"__airflow_xcom_result_empty__":
Review Comment:
Is there a reason that you check for being a string? Can the K8s API return
anything else than `Optional[str]`?
```suggestion
if result and result.rstrip() != "__airflow_xcom_result_empty__":
```
##########
airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -562,11 +577,38 @@ def extract_xcom(self, pod: V1Pod) -> str:
resp,
f"if [ -s {PodDefaults.XCOM_MOUNT_PATH}/return.json ]; then
cat {PodDefaults.XCOM_MOUNT_PATH}/return.json; else echo
__airflow_xcom_result_empty__; fi", # noqa
)
- self._exec_pod_command(resp, "kill -s SIGINT 1")
+ if isinstance(result, str) and result.rstrip() !=
"__airflow_xcom_result_empty__":
+ try:
+ json.loads(result)
+ except Exception as ex:
+ raise ex
Review Comment:
As you raise the exception you receive anyway, you can skip the overhead of
the whole try/catch block
```suggestion
json.loads(result)
```
##########
airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -544,6 +544,21 @@ def await_xcom_sidecar_container_start(self, pod: V1Pod)
-> None:
def extract_xcom(self, pod: V1Pod) -> str:
"""Retrieves XCom value and kills xcom sidecar container."""
+ try:
+ result = self.extract_xcom_json(pod)
+ return result
+ except Exception as ex:
+ raise ex
Review Comment:
Following
https://docs.python.org/3/tutorial/errors.html#defining-clean-up-actions this
is not needed - or is there a reason that you re-raise the same?
```suggestion
```
##########
airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -562,11 +577,38 @@ def extract_xcom(self, pod: V1Pod) -> str:
resp,
f"if [ -s {PodDefaults.XCOM_MOUNT_PATH}/return.json ]; then
cat {PodDefaults.XCOM_MOUNT_PATH}/return.json; else echo
__airflow_xcom_result_empty__; fi", # noqa
)
- self._exec_pod_command(resp, "kill -s SIGINT 1")
+ if isinstance(result, str) and result.rstrip() !=
"__airflow_xcom_result_empty__":
Review Comment:
...or you could revert the order, check for `None`as below first, otherwise
try to `json.loads()` afterwards?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
