-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Migrate Edge calls for Worker to FastAPI part 2 - Logs routes #44330
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate Edge calls for Worker to FastAPI part 2 - Logs routes #44330
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 8 out of 23 changed files in this pull request and generated no suggestions.
Files not reviewed (15)
- docs/spelling_wordlist.txt: Language not supported
- providers/src/airflow/providers/edge/CHANGELOG.rst: Language not supported
- providers/tests/edge/models/test_edge_logs.py: Evaluated as low risk
- providers/src/airflow/providers/edge/cli/api_client.py: Evaluated as low risk
- providers/src/airflow/providers/edge/cli/edge_command.py: Evaluated as low risk
- providers/src/airflow/providers/edge/models/edge_logs.py: Evaluated as low risk
- providers/src/airflow/providers/edge/models/edge_worker.py: Evaluated as low risk
- providers/src/airflow/providers/edge/provider.yaml: Evaluated as low risk
- providers/src/airflow/providers/edge/executors/edge_executor.py: Evaluated as low risk
- providers/src/airflow/providers/edge/worker_api/routes/rpc_api.py: Evaluated as low risk
- providers/src/airflow/providers/edge/worker_api/app.py: Evaluated as low risk
- providers/tests/edge/cli/test_edge_command.py: Evaluated as low risk
- providers/src/airflow/providers/edge/init.py: Evaluated as low risk
- providers/src/airflow/providers/edge/worker_api/routes/_v2_compat.py: Evaluated as low risk
- providers/src/airflow/providers/edge/worker_api/routes/logs.py: Evaluated as low risk
Comments skipped due to low confidence (4)
providers/src/airflow/providers/edge/worker_api/routes/_v2_routes.py:98
- [nitpick] The comment should be more descriptive to explain why a string is used for map_index and how it should be converted to an integer.
map_index: str, # Note: Connexion can not have negative numbers in path parameters, use string therefore
providers/src/airflow/providers/edge/worker_api/routes/_v2_routes.py:126
- Add validation to ensure log_chunk_time is a valid datetime string.
log_chunk_time=body["log_chunk_time"]
providers/src/airflow/providers/edge/worker_api/routes/worker.py:60
- The error message could be more descriptive to help the user understand why their request was rejected.
raise HTTPException(status.HTTP_400_BAD_REQUEST, f"Edge Worker runs on Airflow {airflow_on_worker} and the core runs on {airflow_version}. Rejecting access due to difference.")
providers/src/airflow/providers/edge/worker_api/routes/worker.py:159
- The
Stats.incrmethod is called twice with different parameters. This might be redundant and could be optimized.
Stats.incr(f"edge_worker.heartbeat_count.{worker_name}", 1, 1)
Stats.incr("edge_worker.heartbeat_count", 1, 1, tags={"worker_name": worker_name})
8af06e0 to
74f099c
Compare
a4e286d to
ba54d4c
Compare
providers/src/airflow/providers/edge/worker_api/routes/_v2_routes.py
Outdated
Show resolved
Hide resolved
ba54d4c to
7ff228f
Compare
1fd46a2 to
2311429
Compare
|
Re-reviewed... Looks good :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 14 changed files in this pull request and generated 2 suggestions.
Files not reviewed (9)
- providers/src/airflow/providers/edge/CHANGELOG.rst: Language not supported
- providers/src/airflow/providers/edge/openapi/edge_worker_api_v1.yaml: Language not supported
- providers/tests/edge/models/test_edge_logs.py: Evaluated as low risk
- providers/src/airflow/providers/edge/init.py: Evaluated as low risk
- providers/src/airflow/providers/edge/cli/edge_command.py: Evaluated as low risk
- providers/src/airflow/providers/edge/worker_api/app.py: Evaluated as low risk
- providers/tests/edge/cli/test_edge_command.py: Evaluated as low risk
- providers/src/airflow/providers/edge/models/edge_logs.py: Evaluated as low risk
- providers/src/airflow/providers/edge/provider.yaml: Evaluated as low risk
…#44330) * Migrate Edge calls for Worker to FastAPI 2 - Logs route * Review feedback, use SessionDep from FastAPI * Fix pytest
Follow-up PR as incremental part of #44311
Note: Only the last commit is the relevant change, the first commit is from #44311
To prepare EdgeWorker to be independent of AIP-44 Internal API, this PR is the third step in adding/migrating to FastAPI. The calls to "Logs" API to (1) get log path and (2) push log chunks are now real REST API calls, not using internal API.
I would separate the other internal API calls to follow-up PRs as this is already quite large. Especially cause for ongoing Airflow 2.10 Connexion API + Swagger manually need to be generated whereas the main workstream for Airflow 3 uses FastAPI.