Better support for Boto Waiters #28236

ferruzzi · 2022-12-08T20:11:28Z

This PR cleans up a bit of tech debt in the EKS operators. When I wrote them I didn't know what boto waiters were. A Boto waiter is similar to an Ariflow Sensor but the delay, backoff, and max-retries is all handled server-side by the service. I've replaced a bunch of my custom waiter code with official boto waiters, added the infrastructure to allow us to write custom waiters, along with documentation and an example.

This should make future AWS sensors much easier and cleaner to write. We may want to go through the existing sensors and convert them over at some point. This also opens up an interesting idea of using a custom waiter in a TaskFlow method in place of a sensor. Who knows.

Rendered version of the README is here for easier review.

airflow/providers/amazon/aws/waiters/base_waiter.py

Taragolis · 2022-12-08T20:48:57Z

airflow/providers/amazon/aws/waiters/base_waiter.py

@@ -0,0 +1,37 @@
+# Licensed to the Apache Software Foundation (ASF) under one


I thought BatchClient already implements some kind of waiters (I personally never use it in this hook)

airflow/providers/amazon/aws/hooks/batch_waiters.json

airflow/providers/amazon/aws/hooks/batch_waiters.py

Yes? Looks like it. I've never used that one and it's not something you could inherit and use for other services.

I mean a good point one day also migrate batch waiters to generic solution

Yeah, this would lay the framework for that and adding whatever other custom waiters that folks want to make. The more I look at it, the more I think it'll either simplify or replace a lot of Sensors as well, which wasn't the intended purpose but a very nice side effect.

Taragolis · 2022-12-08T20:50:46Z

airflow/providers/amazon/aws/hooks/eks.py

+    def get_waiter(self, waiter_name):
+        return EksBotoWaiter(client=self.conn).waiter(waiter_name)


Just a thought: "Is it possible to make it part of base AWS hook?"

Maybe, but then you'd have to pass it the waiter model every time too, right?

It just an idea.

It could be some optional argument of hook and if it None than use predefined model for hook (e.g. from waiters directory) if defined than use this model.

On the other hand a lot of boto3 clients doesn't have waiters so it might be add additional complexity if it included in base hook

Maybe possible to work some magic.... if every service's custom waiter model config is in ~/aws/waiters/{service}.json then the service name could be extracted from the hook

Just one additional thing by default there are limited files which included in resulting wheel/sdist package, and json files do not add by default.

So we also need updates in this file:

airflow/dev/provider_packages/MANIFEST_TEMPLATE.in.jinja2

Lines 25 to 30 in 5fad8a4

{% if PROVIDER_PACKAGE_ID == 'amazon' %}

include airflow/providers/amazon/aws/hooks/batch_waiters.json

{% elif PROVIDER_PACKAGE_ID == 'google' %}

include airflow/providers/google/cloud/example_dags/*.yaml

include airflow/providers/google/cloud/example_dags/*.sql

For avoid the situation which happen with k8s provider: #27451

Ohhhhh, thank you. I was unaware of that. I had a chat with Vincent yesterday and he suggested another change so I should have this updated today. Sorry for the delay.

The file you linked at dev/provider_packages/MANIFEST_TEMPLATE.in.jinja2 has a message in it saying

# NOTE! THIS FILE IS AUTOMATICALLY GENERATED AND WILL BE # OVERWRITTEN WHEN PREPARING PACKAGES. # IF YOU WANT TO MODIFY IT, YOU SHOULD MODIFY THE TEMPLATE # `MANIFEST_TEMPLATE.py.jinja2` IN the `provider_packages` DIRECTORY

but there is no file by that name. Do you know if that's an old warning that should be removed/ignored?

ferruzzi:~/workplace/airflow$ find . -name MANIFEST_TEMPLATE* ./dev/provider_packages/MANIFEST_TEMPLATE.in.jinja2

This comment added to generated MANIFEST.in into the provider package.

I thought it some mistake and it should be MANIFEST_TEMPLATE.in.jinja2 instead of MANIFEST_TEMPLATE.py.jinja2.

cc: @potiuk

I changed MANIFEST_TEMPLATE.in.jinja2. If that's not right, I can fix it. I'll need to rebase before this gets merged anyway.

Taragolis

Looking good

ferruzzi · 2022-12-12T02:59:46Z

Looks like it is all passing but wants a rebase again. I'll give it a nudge.

…no longer modified Add unit testing Update README

ferruzzi · 2022-12-12T22:43:17Z

@Taragolis - You have the power now! Do eeeetttt! :P

ferruzzi requested a review from eladkal as a code owner December 8, 2022 20:11

boring-cyborg bot added area:providers provider:amazon AWS/Amazon - related issues labels Dec 8, 2022

Taragolis reviewed Dec 8, 2022

View reviewed changes

ferruzzi requested review from potiuk, ashb and jedcunningham as code owners December 9, 2022 22:21

ferruzzi force-pushed the ferruzzi/eks-waiters branch 3 times, most recently from 391e883 to 0d25b1e Compare December 11, 2022 08:24

Taragolis approved these changes Dec 11, 2022

View reviewed changes

ferruzzi added 3 commits December 11, 2022 18:59

Add waiters to EKS Operators

b5c65dd

Custom waiter setup

2d97536

Move the heavy lifting into base hook - Individual service hooks are …

617665d

…no longer modified Add unit testing Update README

ferruzzi force-pushed the ferruzzi/eks-waiters branch from 0d25b1e to 617665d Compare December 12, 2022 02:59

ferruzzi closed this Dec 12, 2022

ferruzzi reopened this Dec 12, 2022

Taragolis merged commit 27569a8 into apache:main Dec 12, 2022

eladkal mentioned this pull request Jan 2, 2023

Status of testing Providers that were prepared on January 02, 2023 #28679

Closed

84 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better support for Boto Waiters #28236

Better support for Boto Waiters #28236

ferruzzi commented Dec 8, 2022

Taragolis Dec 8, 2022

ferruzzi Dec 8, 2022

Taragolis Dec 9, 2022

ferruzzi Dec 9, 2022

Taragolis Dec 8, 2022

ferruzzi Dec 8, 2022

Taragolis Dec 8, 2022 •

edited

Loading

Taragolis Dec 8, 2022

ferruzzi Dec 8, 2022

Taragolis Dec 9, 2022

ferruzzi Dec 9, 2022

ferruzzi Dec 9, 2022 •

edited

Loading

Taragolis Dec 9, 2022

ferruzzi Dec 9, 2022

Taragolis left a comment

ferruzzi commented Dec 12, 2022

ferruzzi commented Dec 12, 2022

		@@ -0,0 +1,37 @@
		# Licensed to the Apache Software Foundation (ASF) under one

		def get_waiter(self, waiter_name):
		return EksBotoWaiter(client=self.conn).waiter(waiter_name)


	{% if PROVIDER_PACKAGE_ID == 'amazon' %}
	include airflow/providers/amazon/aws/hooks/batch_waiters.json
	{% elif PROVIDER_PACKAGE_ID == 'google' %}
	include airflow/providers/google/cloud/example_dags/*.yaml
	include airflow/providers/google/cloud/example_dags/*.sql

Better support for Boto Waiters #28236

Better support for Boto Waiters #28236

Conversation

ferruzzi commented Dec 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Taragolis Dec 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ferruzzi Dec 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Taragolis left a comment

Choose a reason for hiding this comment

ferruzzi commented Dec 12, 2022

ferruzzi commented Dec 12, 2022

Taragolis Dec 8, 2022 •

edited

Loading

ferruzzi Dec 9, 2022 •

edited

Loading