Introduce a base class for aws triggers #32274
Conversation
boring-cyborg
bot
added
area:providers
provider:amazon
vandonr-amz
force-pushed
the
vandonr/deferrable
branch
from
23e1322 to
8814192
Compare
vandonr-amz
changed the title
| wait( | ||
| waiter=self.get_waiter("query_complete"), | ||
| waiter_delay=sleep_time or self.sleep_time, | ||
| waiter_delay=self.sleep_time if sleep_time is None else sleep_time, |
There was a problem hiding this comment.
this is a somewhat unrelated fix that allows specifying a sleep time of 0 in unit tests. Without this, athena unit tests were taking 30s each
There was a problem hiding this comment.
This is fine, but you can also just mock the sleep function no?
There was a problem hiding this comment.
I think just setting sleep_time=0 is sooo much simpler & cleaner & easier to read
| poll_interval: int, | ||
| max_attempt: int, | ||
| waiter_delay: int, | ||
| waiter_max_attempts: int, |
There was a problem hiding this comment.
this trigger was added in #32186 merged on June 27th, last provider release was on June 20th, so this breaking change is OK.
| compute_env_arn: str | None = None, | ||
| poll_interval: int = 30, | ||
| max_retries: int = 10, | ||
| compute_env_arn: str, | ||
| waiter_delay: int = 30, | ||
| waiter_max_attempts: int = 10, |
There was a problem hiding this comment.
this trigger was added in #32036 merged on June 27th, last provider release was on June 20th, so this breaking change is OK.
| class ClusterWaiterTrigger(BaseTrigger): | ||
| class ClusterActiveTrigger(AwsBaseWaiterTrigger): |
There was a problem hiding this comment.
this trigger was added in #31881 merged on June 23rd, last provider release was on June 20th, so this breaking change is OK.
| class EksNodegroupTrigger(BaseTrigger): | ||
| class EksCreateNodegroupTrigger(AwsBaseWaiterTrigger): |
There was a problem hiding this comment.
this trigger was added in #32165 merged on June 26th, last provider release was on June 20th, so this breaking change is OK.
| waiter_mock.side_effect = WaiterError("name", "reason", {}) | ||
|
|
||
| trigger = AthenaTrigger("query_id", 0, 5, None) | ||
| def test_serialize_recreate(self): |
There was a problem hiding this comment.
This is probably best viewed in side-by-side diff. I removed existing tests because there is no logic anymore in individual triggers.
Instead, I'm testing the only thing that can be broken, which is the serialization/deserialization.
To do that, I do a cycle of serialize-deserialize-reserialize and I compare the serialized data. It'd probably be better to compare the instances, but at least comparing the serialized output can be done with a simple ==
I copy-pasted the same test for all triggers inheriting from the base, because I think it's better to have it in their respective files ? It could also be a parametrized test with many cases in test_base_trigger to avoid the code duplication, open to hear opinion about it.
vincbeck
left a comment
vincbeck
left a comment
There was a problem hiding this comment.
A simple nit but overall I love it!
|
|
||
|
|
||
| class EksNodegroupTrigger(BaseTrigger): | ||
| class EksCreateNodegroupTrigger(AwsBaseWaiterTrigger): |
There was a problem hiding this comment.
Why are you making a separate trigger for create/delete here? Is it no longer possible to use a generic Trigger if the responsibility is to just poll for a particular state (depending on waiter_name)?
There was a problem hiding this comment.
I remember you said back in the day that you preferred separate triggers so that the status/failure messages can be more descriptive ;)
Also, given the lower footprint of triggers created this way, I think it's ok to have specific triggers for each thing.
There was a problem hiding this comment.
Ah, I ended up adopting your method haha. Alright, so just to be clear, I am going to be creating a separate Trigger for each operator again.
Also, given the lower footprint of triggers created this way, I think it's ok to have specific triggers for each thing.
Definitely. Big +1 for reducing the repeated code.
| return EcsHook(aws_conn_id=self.aws_conn_id, region_name=self.region_name) | ||
|
|
||
|
|
||
| class ClusterInactiveTrigger(AwsBaseWaiterTrigger): |
There was a problem hiding this comment.
I may be over-complicating this, but it looooks like the only difference between ClusterActiveTrigger and ClusterInactiveTrigger is the waiter_name value and the two message. In which case, why not have them both inherit a ClusterStatusTrigger which accepts those three values and drop all the repetition?
Same below where you split other Triggers out into status-specific ones.
There was a problem hiding this comment.
the status/failure messages change as well.
But you have the same comment as syed here #32274 (comment)
Co-authored-by: D. Ferruzzi <[email protected]>
| **kwargs: Any, | ||
| poll_interval: int | None = None, # deprecated | ||
| waiter_delay: int = 30, | ||
| waiter_max_attempts: int = 600, |
There was a problem hiding this comment.
is this supposed to be 60?
There was a problem hiding this comment.
it's supposed to be ∞ lol
we don't really have a clear posture on this, some triggers have sensible max attempts values, some others wait forever...
What I don't like with setting a "low" value like 60 is that if the user sets the poll interval to 1 for instance, it ends in 1 minute, which might be annoying.
Overall, I think it should be a really high value, and users should set it themselves if they care.
There was a problem hiding this comment.
I think a good compromise is to keep it the same as the default one used by boto. That way, we don't inadvertently mess things up for users who might be relying on a Task to fail after a certain amount of time, because they didn't change the default value
There was a problem hiding this comment.
ok, but in this particular case, the existing behavior was to wait forever, so....
syedahsn
left a comment
syedahsn
left a comment
There was a problem hiding this comment.
Some minor comments, but overall, looks really good.
ferruzzi
left a comment
ferruzzi
left a comment
There was a problem hiding this comment.
Looks like all of my comments were addressed.
|
this can be merged as is only if #32389 does not proceed. |
|
Amazon provider package is excluded from RC2, therefore we can merge it. See #32389 |
5 participants
There has been a lot of triggers added recently, and they all reuse more or less the same code, with some variations.
This leads to many (small) inconsistencies in trigger code, and also in a lot of duplicated code.
We can package all this in a single base class and reuse it as much as possible, which is what I'm doing here.
Adapting all triggers to this base class is also a good way to resolve some of the inconsistencies.
REVIEWERS:
base_trigger.pycc @syedahsn @ferruzzi @vincbeck