eladkal commented on code in PR #34381:
URL: https://github.com/apache/airflow/pull/34381#discussion_r1327979482
##########
airflow/providers/amazon/aws/executors/ecs/README.md:
##########
@@ -0,0 +1,196 @@
+<!--
Review Comment:
If you consider this alphawe should this executor as experimental.
This will be helpful with the ability to apply breaking changes without
doing major releases.
So my request here is to add `executors.rst` to the amazon provider with 1
paragraph that mentions this new executor and that it is experimental.
##########
airflow/settings.py:
##########
@@ -578,6 +578,10 @@ def initialize():
executor_constants.CELERY_KUBERNETES_EXECUTOR,
executor_constants.LOCAL_KUBERNETES_EXECUTOR,
}
+
+# Executors can set this to true to configure logging correctly for
+# containerized executors.
+IS_EXECUTOR_CONTAINER = bool(os.environ.get("AIRFLOW_IS_EXECUTOR_CONTAINER",
""))
Review Comment:
same question as on the `airflow/utils/log/logging_mixin.py`
##########
airflow/utils/log/logging_mixin.py:
##########
@@ -209,9 +209,9 @@ def __init__(self, stream):
@property
def stream(self):
"""Returns current stream."""
- from airflow.settings import IS_K8S_EXECUTOR_POD
+ from airflow.settings import IS_EXECUTOR_CONTAINER, IS_K8S_EXECUTOR_POD
- if IS_K8S_EXECUTOR_POD:
+ if IS_K8S_EXECUTOR_POD or IS_EXECUTOR_CONTAINER:
Review Comment:
This is core change.
two issues:
1. Is it possible to extract the core changes to separate PR and merge them
before this one? (will make release manager life easier)
2. can you clarify how the executor going to work for users who have Airflow
version that does not include these core changes? (The question here is if this
provider needs min Airflow version that include these core changes)
--
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]
