Skip to content

Conversation

@rajaths010494
Copy link
Contributor

This PR donates the following BatchOperator deferrable developed in astronomer-providers repo to apache airflow.

@rajaths010494
Copy link
Contributor Author

#30489
cc @potiuk

@rajaths010494 rajaths010494 force-pushed the batch_operator_async branch from ba40ff3 to e3d8a79 Compare April 6, 2023 04:45
@potiuk potiuk added the full tests needed We need to run full set of tests for this PR to merge label Apr 6, 2023
@potiuk potiuk closed this Apr 6, 2023
@potiuk potiuk reopened this Apr 6, 2023
@potiuk
Copy link
Member

potiuk commented Apr 6, 2023

Added label. Closed and reopened to rebuild for all python versions

@pankajkoti
Copy link
Member

@vincbeck will you please be able to review this PR? I see you had reviewed #29300 but then it was reverted with #30489 and this is another attempt at adding the deferrable mode to BatchOperator.

@vincbeck
Copy link
Contributor

There are multiple work in parallel to add deferrable operators in Amazon provider package. I do not think there is one better way than the other but I definitely think we should all follow the same pattern for consistency purposes. @syedahsn has been working on adding support for deferrable operators in Amazon provider package in #30032. For instance, he does not create a separate hook for all async operations which I agree with because, I think, having two hooks per service (one for sync operations, one for async operations) would be adding too many files and end up making the amazon provider package harder to maintain.

As a result, I would try to follow the pattern initiated in #30032 while in the meantime we should also try to merge ASAP #30032 to avoid this kind of situation again.

@pankajkoti
Copy link
Member

@vincbeck Thanks Vincent for the visibility on #30032. On our(Astronomer's) end too, we're building/donating quite a few deferrable operators across various providers and would like to also see if most people agree to the pattern, then we could use this pattern not only for Amazon providers but for other providers too. Just curious to know if there was a prior discussion somewhere regarding following this pattern.

I see that we're still keeping the AwsBaseAsyncHook in the hooks in the PR which this PR leverages. I would like to create another PR for the cleanup later but for now, would request that in the meantime to review the other work if it looks good.

cc: @pankajastro @phanikumv @kaxil 👆🏽 Please take a look at the pattern of using a single class for the hook with deferrable param for its methods and would appreciate your thoughts on it.

@vincbeck
Copy link
Contributor

Just curious to know if there was a prior discussion somewhere regarding following this pattern.

Not that I am aware of. I think the discussion actually happened in #30032

@potiuk
Copy link
Member

potiuk commented Apr 24, 2023

I just merged #30032 - so maybe that's a good time to align the approach @vincbeck @rajaths010494. Similar discussions happened befor when Astronomer was donating deferrable operators to Google Provider so maybe the learnings from those discussions and agreening on how to approahc Amazon provider coould be done here as well cc: @kaxil

@syedahsn
Copy link
Contributor

In #30032, I added a README.md that goes over our approach for writing deferrable operators for AMPP. Feel free to go over that document, and let me know if something is missing. I'd love to help out if you're running into any issues!

@pankajkoti
Copy link
Member

@pankajastro as discussed please check the approach mentioned here.

@pankajastro
Copy link
Member

@pankajastro as discussed please check the approach mentioned here.

@pankajkoti @syedahsn will appreciate your feedback on #30865

@pankajkoti
Copy link
Member

We can close this in favour of #30865

cc: @potiuk

@potiuk potiuk closed this Apr 28, 2023
@pankajastro pankajastro deleted the batch_operator_async branch August 5, 2023 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers full tests needed We need to run full set of tests for this PR to merge kind:documentation provider:amazon AWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants