potiuk commented on a change in pull request #20470:
URL: https://github.com/apache/airflow/pull/20470#discussion_r776326780
##########
File path: airflow/dag_processing/processor.py
##########
@@ -149,8 +149,8 @@ def _run_file_processor(
try:
# redirect stdout/stderr to log
- with redirect_stdout(StreamLogWriter(log, logging.INFO)),
redirect_stderr(
- StreamLogWriter(log, logging.WARN)
+ with redirect_stdout(StreamLogWriter(log, logging.INFO)), (
+ redirect_stderr(StreamLogWriter(log, logging.WARN))
), Stats.timer() as timer:
Review comment:
> Although I’m not sure why we need `encoding` exactly; maybe we could
remove this test entirely?
I looked at the history and the reason is that some of the internals of
subprocess use stderr.encoding and stdout.enconding and we are effectvely
replace stderr and stdout with StreamLogWriter (which has other bad
consequences like infinte redirect when someone wants to also log stufff to
stderr/stdout - buit this is a different topic - #20500 )
This is an excerpt from subprcess.py where it woudl fail:
```
# Translate newlines, if requested.
# This also turns bytes into strings.
if self.text_mode:
if stdout is not None:
stdout = self._translate_newlines(stdout,
self.stdout.encoding,
self.stdout.errors)
if stderr is not None:
stderr = self._translate_newlines(stderr,
self.stderr.encoding,
self.stderr.errors)
return (stdout, stderr)
```
On the other hand - we should not really IMHO set it to None (though it
results in "utf-8" being used in `decode` of bytes) but likely with the
`sys.getdefaultencoding()`
--
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]
