hussein-awala commented on code in PR #31663:
URL: https://github.com/apache/airflow/pull/31663#discussion_r1248221321
##########
airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -75,6 +76,12 @@ class PodPhase:
terminal_states = {FAILED, SUCCEEDED}
+class ContainerNames:
+ """Possible container names for airflow."""
+
+ XCOM_CONTAINER = "airflow-xcom-sidecar"
+
+
Review Comment:
I didn't understand the need of this class, this value is already available
in `PodDefaults.SIDECAR_CONTAINER_NAME` which we use in the other methods, it's
better to use it in case we change its name in the future
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -342,6 +348,9 @@ def __init__(
self.cluster_context = cluster_context
self.reattach_on_restart = reattach_on_restart
self.get_logs = get_logs
+ self.container_logs = container_logs
+ if base_container_name and self.container_logs ==
KubernetesPodOperator.BASE_CONTAINER_NAME:
Review Comment:
Why do you think that `self.container_logs ==
KubernetesPodOperator.BASE_CONTAINER_NAME` is not sufficient?
##########
airflow/providers/cncf/kubernetes/utils/pod_manager.py:
##########
@@ -412,14 +431,78 @@ def consume_logs(
)
time.sleep(1)
+ def fetch_requested_container_logs(
Review Comment:
I think that we need to use the provided `post_termination_timeout` in this
method as we did with `fetch_container_logs`
--
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]
