As discussed in https://github.com/apache/airflow/pull/29014#discussion_r1073723719 , we might want to follow the below generic rules(similar to what Pankaj mentioned)
1. When wait_for_termination=True and deferrable=False, we submit and "poll" on the worker 2. When wait_for_termination=True and deferrable=True, we submit and "defer" using Triggerer 3. When wait_for_termination=False and deferrable=False, we only submit 4. When wait_for_termination=False and deferrable=True, we only submit and no deferrable takes place <--- this is where we should warn saying "Deferrable=True" does not have any effect as wait_for_termination=False Note:-For few of the operators, i've seen wait_for_completion param and for few others I've seen wait_for_termination. I think we need to streamline these wait_for* parameters as well On Fri, Jul 21, 2023 at 5:28 PM Pankaj Koti <pankaj.k...@astronomer.io.invalid> wrote: > I would like to propose the following order of precedence: > > 1. If wait_for_completion=False, it should ignore what the > default_deferrable is set to > and should not wait for completion whether synchronously or > asynchronously(deferral) > 2. if wait_for_completion=True, deferrable=True (maybe set explicitly or > via default deferral), trigger deferral and poll async using the triggerer > 3. if wait_for_completion=True, deferrable=False(maybe set explicitly or > via default deferral or default value), poll synchronously. > > In my opinion, generally, "wait_*" params should take precedence over > default_deferral. > > Regards, > > > > Pankaj Koti > > *Senior Software Engineer, *OSS Engineering Team. > Location: Pune, India > > Timezone: Indian Standard Time (IST) > > Email: pankaj.k...@astronomer.io > > Mobile: +91 9730079985 > > > On Fri, Jul 21, 2023 at 4:25 PM Wei Lee <weilee...@gmail.com> wrote: > > > Hi, > > > > Avoiding unexpected behavior is one of the reasons we set the default > > default_deferrable to False. We want to minimize the impact on the users. > > For those who didn't use this feature, everything should work as it used > > to. Those who opt to enable it should know it might change the behavior > of > > existing DAGs. But yes, we can add these checks to mitigate the potential > > surprise. I'm now working on a list of all the operators that might be > > affected. As the changes might take time, we might want to add some > > descriptions in the "default_deferrable" section that this might happen? > > > > Best, > > Wei > > > > > On Jul 21, 2023, at 3:07 AM, Jarek Potiuk <ja...@potiuk.com> wrote: > > > > > > Do we know how many of those affected operators have "wait=False" by > > > default? I think that should help us to determine the action. The > > > "default_deferrable" is an option that has not been advertised (and it > is > > > not yet in config published in any released airflow version, so it is > not > > > yet "officially released" - even if some providers with > > default_deferrable > > > have been released already. So we still have chance to make decision > that > > > is going to be announced in 2.7.0. We can change slightly name of the > > > configuration parameter before 2.7.0, and release new providers with > the > > > support for new parameter, so that someone will not trigger it > > accidentally > > > for those providers with different behaviour from earlier provider > > > versions. That would be enough of "compatibility" protection I think, > > > > > > And yes - changing the behaviour from no wait to wait is undesirable. > > > > > > J. > > > > > > > > > On Tue, Jul 18, 2023 at 12:50 AM Vandon, Raphael > > <vand...@amazon.com.invalid> > > > wrote: > > > > > >> Hello, > > >> This is a repost from slack as Jarek suggested this ML would be a > better > > >> place to discuss it. > > >> > > >> I have a concern with the introduction of a config to control the > > default > > >> value for deferrable (https://github.com/apache/airflow/pull/31712) > > >> A good portion of operators had a parameter to control whether they > wait > > >> or not (wait_until_finished, wait_for_completion, > wait_for_termination, > > or > > >> just wait). > > >> I think most implementations of deferrable treat it as an override for > > >> this setting (i.e. if deferrable = True then the value of wait does > not > > >> matter anymore) > > >> My concern is that if users have operators that do not wait (either > > >> because the default value for wait was False, or they set it manually > to > > >> False), and they set default_deferrable=True, then all their operators > > will > > >> start waiting, which might not be what they expect. > > >> I think what’d be nice is if setting the default only > > >> changed how operators work (deferring instead of waiting in the > worker), > > >> not whether they wait or not. > > >> It might be too late to change this because there has been a release > > since > > >> then, but we could say that: > > >> • if deferrable is manually set to True, we always wait & defer > > >> • else if wait_for_completion is False (default value, or set > > manually), > > >> we don’t wait > > >> • in other cases (wait for completion True and no set value for > > >> deferrable), we rely on the config. > > >> That way, users setting this conf wouldn’t see their dags suddenly > > taking > > >> more time because they start waiting where they weren’t before. > > >> > > >> This would impact any operator that has a legacy mechanism to wait > > >> synchronously and a deferrable mode. > > >> Especially those where the default value to wait was False, like > > >> RedshiftCreateClusterOperator for instance (but there are others). > > >> > > >> > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > > For additional commands, e-mail: dev-h...@airflow.apache.org > > > > >
