You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2022/11/10 13:52:19 UTC
[GitHub] [airflow] ashb opened a new pull request, #27591: Fix double logging with some task logging handler
ashb opened a new pull request, #27591:
URL: https://github.com/apache/airflow/pull/27591
A previous change to fix disappearing log messages turned on propagation for the `airflow.task` logger, and disabled it again after `set_context()` was called, but only if that function returned a special sentinel value.
For the "core" task log handlers we returned them, but some providers weren't "correctly" subclassing and weren't returning this sentinel value.
The fix here is to change the logic from disable only on special value to disable by default and maintain propagation on special value; this means that if a handler doesn't return the value from `super()` (or if they don't even subclass the default handler) propagation will still be disabled by default.
Fixes #27345
--
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
[GitHub] [airflow] ashb commented on a diff in pull request #27591: Fix double logging with some task logging handler
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #27591:
URL: https://github.com/apache/airflow/pull/27591#discussion_r1019294332
##########
airflow/utils/log/logging_mixin.py:
##########
@@ -18,18 +18,35 @@
from __future__ import annotations
import abc
+import enum
import logging
import re
import sys
from io import IOBase
from logging import Handler, Logger, StreamHandler
-from typing import IO
+from typing import IO, cast
# 7-bit C1 ANSI escape sequences
ANSI_ESCAPE = re.compile(r"\x1B[@-_][0-?]*[ -/]*[@-~]")
-# Private: A sentinel object
-DISABLE_PROPOGATE = object()
+
+# Private: A sentinel objects
+class SetContextPropagate(enum.Enum):
+ """:meta private:"""
+
+ # If a `set_context` function wants to _keep_ propagation set on it's logger it needs to return this
+ # special value.
+ MAINTAIN_PROPAGATE = object()
+ # Don't use this one anymore!
+ DISABLE_PROPAGATE = object()
+
+
+def __getattr__(name):
+ if name in ("DISABLE_PROPOGATE", "DISABLE_PROPAGATE"):
Review Comment:
We all knew I couldn't spell right?
(The switch to an Enum was for typing reasons, no easy way of saying "returns none or this _specific_ object type")
--
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
[GitHub] [airflow] potiuk commented on pull request #27591: Fix double logging with some task logging handler
Posted by GitBox <gi...@apache.org>.
potiuk commented on PR #27591:
URL: https://github.com/apache/airflow/pull/27591#issuecomment-1310478331
Nice
--
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
[GitHub] [airflow] ashb merged pull request #27591: Fix double logging with some task logging handler
Posted by GitBox <gi...@apache.org>.
ashb merged PR #27591:
URL: https://github.com/apache/airflow/pull/27591
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #27591: Fix double logging with some task logging handler
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27591:
URL: https://github.com/apache/airflow/pull/27591#discussion_r1019215958
##########
airflow/providers/amazon/aws/log/cloudwatch_task_handler.py:
##########
@@ -61,12 +61,13 @@ def _render_filename(self, ti, try_number):
return super()._render_filename(ti, try_number).replace(":", "_")
def set_context(self, ti):
- super().set_context(ti)
+ ret = super().set_context(ti)
Review Comment:
Since there is a need to fix tests. Should we also add `-> Any` (and ti: TaskInstance in param) in `set_context` method of FileTaskHandler and in all subclasses (and update docstring in set_context() of FileTaskHandler. I tihnk not having it was one of the reasons why return value was missing 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a diff in pull request #27591: Fix double logging with some task logging handler
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #27591:
URL: https://github.com/apache/airflow/pull/27591#discussion_r1019218377
##########
airflow/providers/amazon/aws/log/cloudwatch_task_handler.py:
##########
@@ -61,12 +61,13 @@ def _render_filename(self, ti, try_number):
return super()._render_filename(ti, try_number).replace(":", "_")
def set_context(self, ti):
- super().set_context(ti)
+ ret = super().set_context(ti)
Review Comment:
Whoops, I didn't mean to commit this change.
I'll update the typing 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
[GitHub] [airflow] ashb commented on pull request #27591: Fix double logging with some task logging handler
Posted by GitBox <gi...@apache.org>.
ashb commented on PR #27591:
URL: https://github.com/apache/airflow/pull/27591#issuecomment-1310613492
It appears my PR skills have attrophied to the point where I'm just making silly mistakes :D
--
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
[GitHub] [airflow] feluelle commented on a diff in pull request #27591: Fix double logging with some task logging handler
Posted by GitBox <gi...@apache.org>.
feluelle commented on code in PR #27591:
URL: https://github.com/apache/airflow/pull/27591#discussion_r1019310095
##########
airflow/utils/log/logging_mixin.py:
##########
@@ -18,18 +18,35 @@
from __future__ import annotations
import abc
+import enum
import logging
import re
import sys
from io import IOBase
from logging import Handler, Logger, StreamHandler
-from typing import IO
+from typing import IO, cast
# 7-bit C1 ANSI escape sequences
ANSI_ESCAPE = re.compile(r"\x1B[@-_][0-?]*[ -/]*[@-~]")
-# Private: A sentinel object
-DISABLE_PROPOGATE = object()
+
+# Private: A sentinel objects
+class SetContextPropagate(enum.Enum):
+ """:meta private:"""
+
+ # If a `set_context` function wants to _keep_ propagation set on it's logger it needs to return this
+ # special value.
+ MAINTAIN_PROPAGATE = object()
+ # Don't use this one anymore!
+ DISABLE_PROPAGATE = object()
+
+
+def __getattr__(name):
+ if name in ("DISABLE_PROPOGATE", "DISABLE_PROPAGATE"):
Review Comment:
Looking forward to the depreciation warning hahahaha :D
--
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
[GitHub] [airflow] potiuk commented on a diff in pull request #27591: Fix double logging with some task logging handler
Posted by GitBox <gi...@apache.org>.
potiuk commented on code in PR #27591:
URL: https://github.com/apache/airflow/pull/27591#discussion_r1019291438
##########
airflow/utils/log/logging_mixin.py:
##########
@@ -18,18 +18,35 @@
from __future__ import annotations
import abc
+import enum
import logging
import re
import sys
from io import IOBase
from logging import Handler, Logger, StreamHandler
-from typing import IO
+from typing import IO, cast
# 7-bit C1 ANSI escape sequences
ANSI_ESCAPE = re.compile(r"\x1B[@-_][0-?]*[ -/]*[@-~]")
-# Private: A sentinel object
-DISABLE_PROPOGATE = object()
+
+# Private: A sentinel objects
+class SetContextPropagate(enum.Enum):
+ """:meta private:"""
+
+ # If a `set_context` function wants to _keep_ propagation set on it's logger it needs to return this
+ # special value.
+ MAINTAIN_PROPAGATE = object()
+ # Don't use this one anymore!
+ DISABLE_PROPAGATE = object()
+
+
+def __getattr__(name):
+ if name in ("DISABLE_PROPOGATE", "DISABLE_PROPAGATE"):
Review Comment:
:D
--
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
[GitHub] [airflow] ashb commented on a diff in pull request #27591: Fix double logging with some task logging handler
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #27591:
URL: https://github.com/apache/airflow/pull/27591#discussion_r1019316858
##########
airflow/utils/log/logging_mixin.py:
##########
@@ -18,18 +18,35 @@
from __future__ import annotations
import abc
+import enum
import logging
import re
import sys
from io import IOBase
from logging import Handler, Logger, StreamHandler
-from typing import IO
+from typing import IO, cast
# 7-bit C1 ANSI escape sequences
ANSI_ESCAPE = re.compile(r"\x1B[@-_][0-?]*[ -/]*[@-~]")
-# Private: A sentinel object
-DISABLE_PROPOGATE = object()
+
+# Private: A sentinel objects
+class SetContextPropagate(enum.Enum):
+ """:meta private:"""
+
+ # If a `set_context` function wants to _keep_ propagation set on it's logger it needs to return this
+ # special value.
+ MAINTAIN_PROPAGATE = object()
+ # Don't use this one anymore!
+ DISABLE_PROPAGATE = object()
+
+
+def __getattr__(name):
+ if name in ("DISABLE_PROPOGATE", "DISABLE_PROPAGATE"):
Review Comment:
Luckily in this case I don't think we need one: it was only for a single point release, and it didn't really work in practice
😅
--
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
[GitHub] [airflow] feluelle commented on a diff in pull request #27591: Fix double logging with some task logging handler
Posted by GitBox <gi...@apache.org>.
feluelle commented on code in PR #27591:
URL: https://github.com/apache/airflow/pull/27591#discussion_r1019321447
##########
airflow/utils/log/logging_mixin.py:
##########
@@ -18,18 +18,35 @@
from __future__ import annotations
import abc
+import enum
import logging
import re
import sys
from io import IOBase
from logging import Handler, Logger, StreamHandler
-from typing import IO
+from typing import IO, cast
# 7-bit C1 ANSI escape sequences
ANSI_ESCAPE = re.compile(r"\x1B[@-_][0-?]*[ -/]*[@-~]")
-# Private: A sentinel object
-DISABLE_PROPOGATE = object()
+
+# Private: A sentinel objects
+class SetContextPropagate(enum.Enum):
+ """:meta private:"""
+
+ # If a `set_context` function wants to _keep_ propagation set on it's logger it needs to return this
+ # special value.
+ MAINTAIN_PROPAGATE = object()
+ # Don't use this one anymore!
+ DISABLE_PROPAGATE = object()
+
+
+def __getattr__(name):
+ if name in ("DISABLE_PROPOGATE", "DISABLE_PROPAGATE"):
Review Comment:
But I want to see you suffer 😆 You need to learn from those mistakes 😆
--
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
[GitHub] [airflow] ashb commented on a diff in pull request #27591: Fix double logging with some task logging handler
Posted by GitBox <gi...@apache.org>.
ashb commented on code in PR #27591:
URL: https://github.com/apache/airflow/pull/27591#discussion_r1019375160
##########
airflow/utils/log/logging_mixin.py:
##########
@@ -18,18 +18,35 @@
from __future__ import annotations
import abc
+import enum
import logging
import re
import sys
from io import IOBase
from logging import Handler, Logger, StreamHandler
-from typing import IO
+from typing import IO, cast
# 7-bit C1 ANSI escape sequences
ANSI_ESCAPE = re.compile(r"\x1B[@-_][0-?]*[ -/]*[@-~]")
-# Private: A sentinel object
-DISABLE_PROPOGATE = object()
+
+# Private: A sentinel objects
+class SetContextPropagate(enum.Enum):
+ """:meta private:"""
+
+ # If a `set_context` function wants to _keep_ propagation set on it's logger it needs to return this
+ # special value.
+ MAINTAIN_PROPAGATE = object()
+ # Don't use this one anymore!
+ DISABLE_PROPAGATE = object()
+
+
+def __getattr__(name):
+ if name in ("DISABLE_PROPOGATE", "DISABLE_PROPAGATE"):
Review Comment:
NEVER!
--
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