[GitHub] [airflow] uranusjr commented on a change in pull request #15743: Redirect forked process output to logger

[GitBox <gi...@apache.org>]

uranusjr commented on a change in pull request #15743:
URL: https://github.com/apache/airflow/pull/15743#discussion_r628842968



##########
File path: airflow/task/task_runner/base_task_runner.py
##########
@@ -105,7 +105,8 @@ def deserialize_run_error(self) -> Optional[Union[str, Exception]]:
         """Return task runtime error if its written to provided error file."""
         return load_error_file(self._error_file)
 
-    def _read_task_logs(self, stream):
+    def _stream_reader(self, stream):
+        """Reading from the stream and log into task logs"""
         while True:
             line = stream.readline()
             if isinstance(line, bytes):

Review comment:
       The only place `_read_task_logs()` is called, `stream` is the stdout of a `Popen(universal_newlines=True)`, so it can actually never emit bytes anywhere in the code base. And since `_read_task_logs()` is private, any third-party code that relies on it is subject to break anyway.
   
   I would categorise this as *overly defensive* programming in the original implementation and simply remvoe the `if isinstance(line, bytes)` check entirely. It should not have been written in the first place.




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org