You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "dstandish (via GitHub)" <gi...@apache.org> on 2023/12/14 18:25:06 UTC
[PR] KPO write_logs does not need to use complicated pod logs reader [airflow]
dstandish opened a new pull request, #36228:
URL: https://github.com/apache/airflow/pull/36228
Method `write_logs` was using pod manager method `read_pod_logs` which has some rather complicated logic that it doesn't need, because it does not "follow" the logs and because by the time we come out of deferral, the pod is already done.
By reducing usage of `read_pod_logs`, it makes it easier to hopefully refactor and simplify some of our KPO log fetching logic.
I also make the metheod explicitly private which it should have been anyway.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KPO write_logs does not need to use complicated pod logs reader [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #36228:
URL: https://github.com/apache/airflow/pull/36228#discussion_r1452273737
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -703,22 +703,21 @@ def execute_complete(self, context: Context, event: dict, **kwargs):
remote_pod=pod,
)
- def write_logs(self, pod: k8s.V1Pod):
+ def _write_logs(self, pod: k8s.V1Pod):
Review Comment:
Checked the discussion in https://github.com/apache/airflow/pull/36767, seems about right to me that no one "should" ever override the write_logs method, it is something that should not be changed. Seems like a non breaking change
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -651,22 +651,21 @@ def execute_complete(self, context: Context, event: dict, **kwargs):
remote_pod=pod,
)
- def write_logs(self, pod: k8s.V1Pod):
+ def _write_logs(self, pod: k8s.V1Pod):
try:
- logs = self.pod_manager.read_pod_logs(
- pod=pod,
+ logs = self.client.read_namespaced_pod_log(
+ name=pod.metadata.name,
+ namespace=pod.metadata.namespace,
container_name=self.base_container_name,
+ timestamps=False,
follow=False,
+ _preload_content=False,
)
for raw_line in logs:
line = raw_line.decode("utf-8", errors="backslashreplace").rstrip("\n")
- self.log.info("Container logs: %s", line)
+ self.log.info("[%s] %s", self.base_container_name, line)
except HTTPError as e:
- self.log.warning(
- "Reading of logs interrupted with error %r; will retry. "
- "Set log level to DEBUG for traceback.",
- e,
- )
+ self.log.warning("Reading of logs interrupted with error %r;", e)
Review Comment:
Does it retry? We should mention in the warning in that case.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KPO write_logs does not need to use complicated pod logs reader [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #36228:
URL: https://github.com/apache/airflow/pull/36228#discussion_r1452044862
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -703,22 +703,21 @@ def execute_complete(self, context: Context, event: dict, **kwargs):
remote_pod=pod,
)
- def write_logs(self, pod: k8s.V1Pod):
+ def _write_logs(self, pod: k8s.V1Pod):
Review Comment:
cool yeah I see the discussion in https://github.com/apache/airflow/pull/36767
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KPO write_logs does not need to use complicated pod logs reader [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #36228:
URL: https://github.com/apache/airflow/pull/36228#discussion_r1448031027
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -703,22 +703,21 @@ def execute_complete(self, context: Context, event: dict, **kwargs):
remote_pod=pod,
)
- def write_logs(self, pod: k8s.V1Pod):
+ def _write_logs(self, pod: k8s.V1Pod):
Review Comment:
is this considered a breaking change?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KPO write_logs does not need to use complicated pod logs reader [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #36228:
URL: https://github.com/apache/airflow/pull/36228#discussion_r1451635437
##########
airflow/providers/cncf/kubernetes/operators/pod.py:
##########
@@ -703,22 +703,21 @@ def execute_complete(self, context: Context, event: dict, **kwargs):
remote_pod=pod,
)
- def write_logs(self, pod: k8s.V1Pod):
+ def _write_logs(self, pod: k8s.V1Pod):
Review Comment:
I don't think we need to consider it breaking. This is a method that realistically no one would ever override. It's used in one place. I think of it as "bugfix" that it wasn't marked private.
Really this also connects to something we've discussed before. That our operators should not be published as "base classes" to be extended. I think they should be end products whose behavior is governed by semver but not whose _structure_ is governed by semver. Personally I think we ought to update our "public interface" doc to reflect this.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KPO write_logs does not need to use complicated pod logs reader [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #36228: KPO write_logs does not need to use complicated pod logs reader
URL: https://github.com/apache/airflow/pull/36228
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KPO write_logs does not need to use complicated pod logs reader [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36228:
URL: https://github.com/apache/airflow/pull/36228#issuecomment-1972189993
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org