pankajkoti commented on code in PR #31595:
URL: https://github.com/apache/airflow/pull/31595#discussion_r1209280972
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -301,6 +302,7 @@ def __init__(
base_container_name: str | None = None,
deferrable: bool = False,
poll_interval: float = 2,
+ log_pod_on_failure: bool = False,
Review Comment:
IMO, the default value should be True to keep the existing behaviour as is
and then it could be disabled by explicitly setting it to `False` by the
calling function.
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -225,6 +225,7 @@ class KubernetesPodOperator(BaseOperator):
container name to use.
:param deferrable: Run operator in the deferrable mode.
:param poll_interval: Polling period in seconds to check for the status.
Used only in deferrable mode.
+ :param log_pod_on_failure: If pod template should be logged in a case of
task fail.
Review Comment:
```suggestion
:param log_pod_spec_on_failure: If pod template should be logged in a
case of task fail.
```
or pod_specification perhaps.
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -696,10 +698,11 @@ def cleanup(self, pod: k8s.V1Pod, remote_pod: k8s.V1Pod):
f"Pod {pod and pod.metadata.name} returned exit code "
f"{self.skip_on_exit_code}. Skipping."
)
- raise AirflowException(
- f"Pod {pod and pod.metadata.name} returned a
failure:\n{error_message}\n"
- f"remote_pod: {remote_pod}"
- )
+ raise AirflowException('\n'.join(filter(None, [
+ f"Pod {pod and pod.metadata.name} returned a failure.",
+ error_message,
Review Comment:
I think a new line to separate this would be nicer. Also, previously if the
value was not set, we were converting it to an empty string. Now, we're
converting it to an empty string, would it have some impact?
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -696,10 +698,11 @@ def cleanup(self, pod: k8s.V1Pod, remote_pod: k8s.V1Pod):
f"Pod {pod and pod.metadata.name} returned exit code "
f"{self.skip_on_exit_code}. Skipping."
)
- raise AirflowException(
- f"Pod {pod and pod.metadata.name} returned a
failure:\n{error_message}\n"
- f"remote_pod: {remote_pod}"
- )
+ raise AirflowException('\n'.join(filter(None, [
+ f"Pod {pod and pod.metadata.name} returned a failure.",
+ error_message,
+ f"remote_pod: {remote_pod}" if self.log_pod_on_failure else
None
Review Comment:
I think instead of None, we could add a string here, that displaying pod
specification is disabled.
--
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]
