SameerMesiah97 commented on PR #71653: URL: https://github.com/apache/airflow/pull/71653#issuecomment-5461780144
> I think we really need to be careful when moving away from very well tested and delicate code that all Triggers share (async_wait) in this case and trying to re-implement it. I know you have good intentions and are trying to provide more information after the Trigger returns, but you're opening yourself up to a lot of new code to maintain (and almost duplicate) as well as all the bugs and edge cases that come along with that. After reading your comment, I have realized the core issue is not with the SageMakerTrigger but AwsBaseWaiterTrigger flattening the waiter outcome into success / error, so `SageMakerTrigger `can't preserve states like `Failed` and `Stopped` without taking over the polling loop. What are your thoughts on adding a overridable translation method i.e. `_event_from_exception` to `AwsBaseWaiterTrigger`, which the `SageMakerTrigger` uses to surface more granular `TriggerEvent` payloads? I believe this should preserve centralizied handling of `ThrottlingError`, the polling loop, timeouts etc whilst keeping the state machine for this trigger (and any other AWS trigger) explicit. -- 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]
