Copilot commented on code in PR #52952:
URL: https://github.com/apache/airflow/pull/52952#discussion_r2188489709
##########
providers/edge3/src/airflow/providers/edge3/plugins/edge_executor_plugin.py:
##########
@@ -213,12 +214,18 @@ def change_maintenance_comment(self, worker_name: str):
except AirflowConfigException:
EDGE_EXECUTOR_ACTIVE = False
+# Load the API endpoint only on api-server (Airflow 3.x) or webserver (Airflow
2.x)
+if AIRFLOW_V_3_0_PLUS:
+ RUNNING_ON_APISERVER = sys.argv[1] in ["api-server"] if len(sys.argv) > 1
else False
+else:
+ RUNNING_ON_APISERVER = "gunicorn" in sys.argv[0] and "airflow-webserver"
in sys.argv
Review Comment:
The check `"airflow-webserver" in sys.argv` tests the entire list rather
than its elements; this may not catch the intended argument. Use something like
`any("airflow-webserver" in arg for arg in sys.argv)` for accurate matching.
```suggestion
RUNNING_ON_APISERVER = "gunicorn" in sys.argv[0] and
any("airflow-webserver" in arg for arg in sys.argv)
```
##########
providers/edge3/src/airflow/providers/edge3/plugins/edge_executor_plugin.py:
##########
@@ -213,12 +214,18 @@ def change_maintenance_comment(self, worker_name: str):
except AirflowConfigException:
EDGE_EXECUTOR_ACTIVE = False
+# Load the API endpoint only on api-server (Airflow 3.x) or webserver (Airflow
2.x)
+if AIRFLOW_V_3_0_PLUS:
+ RUNNING_ON_APISERVER = sys.argv[1] in ["api-server"] if len(sys.argv) > 1
else False
+else:
+ RUNNING_ON_APISERVER = "gunicorn" in sys.argv[0] and "airflow-webserver"
in sys.argv
+
Review Comment:
[nitpick] Consider extracting the detection logic and magic strings
(`"api-server"`, `"gunicorn"`, `"airflow-webserver"`) into well-named constants
or a helper function to improve readability and reduce duplication.
```suggestion
API_SERVER_ARG = "api-server"
GUNICORN_PROCESS = "gunicorn"
AIRFLOW_WEBSERVER_ARG = "airflow-webserver"
def _is_running_on_apiserver() -> bool:
"""Determine if the current process is running on the API server."""
if AIRFLOW_V_3_0_PLUS:
return sys.argv[1] == API_SERVER_ARG if len(sys.argv) > 1 else False
return GUNICORN_PROCESS in sys.argv[0] and AIRFLOW_WEBSERVER_ARG in
sys.argv
RUNNING_ON_APISERVER = _is_running_on_apiserver()
```
##########
providers/edge3/src/airflow/providers/edge3/plugins/edge_executor_plugin.py:
##########
@@ -213,12 +214,18 @@ def change_maintenance_comment(self, worker_name: str):
except AirflowConfigException:
EDGE_EXECUTOR_ACTIVE = False
+# Load the API endpoint only on api-server (Airflow 3.x) or webserver (Airflow
2.x)
+if AIRFLOW_V_3_0_PLUS:
+ RUNNING_ON_APISERVER = sys.argv[1] in ["api-server"] if len(sys.argv) > 1
else False
+else:
+ RUNNING_ON_APISERVER = "gunicorn" in sys.argv[0] and "airflow-webserver"
in sys.argv
Review Comment:
[nitpick] Relying on `sys.argv` for component detection can be brittle.
Consider using a dedicated Airflow context or configuration setting (e.g., a
CLI context indicator or environment variable) to determine the runtime
component more reliably.
```suggestion
RUNNING_ON_APISERVER = conf.getboolean("edge", "api_server_mode",
fallback=False)
else:
RUNNING_ON_APISERVER = conf.getboolean("edge", "webserver_mode",
fallback=False)
```
--
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]
