You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Taragolis (via GitHub)" <gi...@apache.org> on 2023/03/11 00:27:52 UTC

[GitHub] [airflow] Taragolis commented on a diff in pull request #30032: Add support for deferrable operators in AMPP

Taragolis commented on code in PR #30032:
URL: https://github.com/apache/airflow/pull/30032#discussion_r1132989959


##########
airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -42,6 +42,7 @@
 import botocore.session
 import requests
 import tenacity
+from aiobotocore.session import AioSession, get_session as async_get_session

Review Comment:
   My main concerns do not changed:
   - `aiobotocore` pin `botocore` and `boto3` for specific version, last version update takes 7 month, if compare with regular boto stuff it have 3-5 updates per week: fixes, new AWS services and etc. So my opinion that this should not be a part of core dependencies, only if we want to make one or two steps back.
   - There was plan to add more consistency (not early than 30 Apr) in hooks and add type hinting by separate existed hooks by https://github.com/apache/airflow/discussions/28560 by separate base hooks. And `aiobotocore.session.AioSession` is not a replacement of `boto3.session.Session`.
   - `aiobotocore` can't work with `moto` and some critical stuff tested by `moto`, e.g. auth and AssumeRole
   -  My opinion still the same, Airflow core not ready for async stuff `¯\_(ツ)_/¯`, all DB queries are use io blocking implementation (the regular one), so obtain credentials from config still required use some hacks, like `asgiref.sync.sync_to_async` which is transform triggerer service to [some kind of sequential executor](https://github.com/apache/airflow/pull/29038#discussion_r1100693885) 
   - Current PR implementation use only blocking io features, can't find any `asyncio` usage.
   
   Personally I would not use veto for this PR, as well as other async/deferrable related stuff. I start to believe that more we add stuff which impact performance degradation more faster we add native asyncio support in core.
   



-- 
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