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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
