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

["syedahsn (via GitHub)" <gi...@apache.org>]

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


##########
airflow/providers/amazon/aws/hooks/base_aws.py:
##########
@@ -72,7 +74,7 @@
 
 class BaseSessionFactory(LoggingMixin):
     """
-    Base AWS Session Factory class to handle boto3 session creation.
+    Base AWS Session Factory class to handle synchronous and async boto session creation.

Review Comment:
    It's true that getting credentials from the database is a blocking io operation, and will hold up the async thread when the connection is made, but I don't think that diminishes the advantages of using deferrable operators too significantly. Just taking the example of creating a Redshift cluster, which takes around 5 minutes to fully provision, if we can free up a worker so that those 5 minutes are used effectively, then the time spent holding up the async thread (which is usually around 0.2 to 0.8 seconds in my experience) is negligible. I do think core airflow can be improved to better support asyncio connections, and it is something that we should make efforts to achieve, but I think it can be done in parallel.
   
   > Except unittest we do not have anything for test this part. Also for obtain initial credentials it is use STS client (from boto3/botocore) which also doesn't use asyncio implementation.
   
   For the unit tests, I was able to work past the sts mocking, and I am in the process of adding more tests. It is slightly more cumbersome to not be able to use moto, but that is the price of using asyncio, and considering the benefits, worth paying in my opinion.
   
   >In addition, AFAIK, some of the part not supported by aiobotocore at all such as Google Federation, SPNEGO/Kerberos and EKS IRSA
   
   This is something that again can be incorporated as the need for these features become apparent. Ideally, the async_conn property is something that we wouldn't encourage users to use on their own, because of the limitations of asyncio. The goal is to provide users with the option of using deferrable operators without them having to deal with asyncio. 
   
   
   >Potentially better idea is extend BaseAsyncSessionFactory rather than BaseSessionFactory
   
   This is an approach that has been taken, and is a valid approach. However, I feel that it leads to large amounts of code duplication, which will become difficult to manage as things progress. I think it is better to include async functionality to BaseSessionFactory, and deal with issues as they come up, so that as things move forward, we work towards a more unified code base, rather than ending up with 2 similar but separate code bases.



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