Fix Comprehend document classifier timing out after 20 minutes - #71969
Fix Comprehend document classifier timing out after 20 minutes#71969ramitkataria wants to merge 2 commits into
Conversation
Classifier training routinely takes 8 to 20+ minutes, but the operator's default waiter budget was 20 x 60s in both sync and deferrable paths, while its trigger and companion sensor default to 75 x 120s; align the operator with them. This also lets the system test Dag drop its wait_for_completion=False workaround, so the test exercises the operator's default wait path.
boring-cyborg
Bot
added
area:providers
provider:amazon
o-nikolas
left a comment
o-nikolas
left a comment
There was a problem hiding this comment.
One could argue that this is a breaking change since the operator will now exhibit a different behaviour than before (Dags may take longer than they usually do). But I think this fits as bug fix since before the timeout needed to be increased otherwise the operation wouldn't have succeeded anyway, which is the ultimate goal.
| wait_for_completion: bool = True, | ||
| waiter_delay: int = 60, | ||
| waiter_max_attempts: int = 20, | ||
| waiter_delay: int = 120, |
There was a problem hiding this comment.
Having the delay be 60 was convenient for reasoning about the total number of minutes, so I'd maybe keep it configured to 60 here. Also, 60*75 leads to a MUCH bigger timeout than before, going from 20m to well over 2 hours. I think that is too big of a jump. Dag runs will be blocked for a long long time unexpectedly as a result.
Maybe double the existing total timeout? so waiter_delay=60 and waiter_max_attempts=40?
There was a problem hiding this comment.
The sensor and trigger are at 75 × 120s and there was 1 run in the last month where the sensor waited for 44 mins. So maybe 50 or 60 mins? Either way, I guess we're leaving the sensor without any changes because that would be an unnecessary breaking change, right?
That merits a warning note at the top of the changelog so can be added to the bugfix release. |
3 participants
Classifier training routinely takes 8 to 20+ minutes, but the operator's default waiter budget was 20 x 60s in both sync and deferrable paths, a cap the training time regularly exceeded. Double it to 40 x 60s. (The companion sensor and trigger allow 75 x 120s, but jumping the operator's implicit wait to 150 minutes would block Dag runs unexpectedly long, so they are intentionally left as is.) This also lets the system test Dag drop its wait_for_completion=False workaround, so the test exercises the operator's default wait path.
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.