Mailing-List: contact dev-help@airflow.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@airflow.incubator.apache.org
MIME-Version: 1.0
In-Reply-To: <CAF+3fpOhaRxNeLwqskNd-1Om6CVMFv9C19rTen1tVRoJSerb2Q@mail.gmail.com>
References: <F20CB0D4-3EF4-4968-9B98-D82D71EE1AFF@gmail.com>
	<CABYbY7e-qu1+UyZyKWZjPfs1Z3OAXX40_p6Acj7Hz8XMSf7ZSw@mail.gmail.com>
	<2DD52EF7-8A30-4B80-940E-9FB119C7F868@gmail.com>
	<CAF+3fpOhaRxNeLwqskNd-1Om6CVMFv9C19rTen1tVRoJSerb2Q@mail.gmail.com>
Date: Fri, 6 May 2016 14:02:25 -0700
Message-ID: <CABYbY7ci6Yb6vuxBHTmb3_H==Nqp3Lpd4U8qHRiY6e1N0icCJA@mail.gmail.com>
Subject: Re: 1.7.1 release blocker: [AIRFLOW-56] Airflow's scheduler can
 "lose" queued tasks
From: Chris Riccomini <criccomini@apache.org>
To: dev@airflow.incubator.apache.org
Content-Type: multipart/alternative; boundary=001a113eae10a0d0f9053232c6ed
archived-at: Fri, 06 May 2016 21:02:32 -0000

--001a113eae10a0d0f9053232c6ed
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Same :)

On Fri, May 6, 2016 at 1:57 PM, Arthur Wiedmer <arthur.wiedmer@gmail.com>
wrote:

> Good, because that's the version we use :)
>
> On Fri, May 6, 2016 at 1:55 PM, Bolke de Bruin <bdbruin@gmail.com> wrote:
>
> > It seems it does: https://dev.mysql.com/doc/refman/5.6/en/select.html
> >
> > Bolke
> >
> > Sent from my iPhone
> >
> > On 6 mei 2016, at 22:52, Chris Riccomini <criccomini@apache.org> wrote:
> >
> > >> Postgres, Mysql >=3D 5.7)
> > >
> > > Curious--does this work in MySQL 5.6?
> > >
> > >> On Fri, May 6, 2016 at 1:36 PM, Bolke de Bruin <bdbruin@gmail.com>
> > wrote:
> > >>
> > >> Dear All,
> > >>
> > >> One of the remaining issues to resolve is Airflow-56: the fact that
> > >> airflow=E2=80=99s scheduler can =E2=80=9Close=E2=80=9D queued tasks.=
 One of the recent changes
> > was
> > >> the hold queue status in the executor instead using the database.
> While
> > >> this goes against the =E2=80=9Cdatabase knows the state=E2=80=9D, it=
 was done (afaik)
> to
> > >> prevent a race condition when a scheduler might actually set the tas=
k
> > twice
> > >> for execution as the task might not have updated its state in the
> > database
> > >> yet. However, if the executor loses its queue for some reason the
> tasks
> > >> that have a state of queued in the database will never get picked up=
.
> > >> Airflow-56 contains a DAG that will exhibit this behavior on clean
> > install
> > >> of 1.7.1rc3 with mysql and local executor.
> > >>
> > >> Jeremiah has worked on
> > >> https://github.com/apache/incubator-airflow/pull/1378, that basicall=
y
> > >> reverts the behavior to using the database (pre 1.7.0), but contains
> > some
> > >> additional fixes. In addition I have asked him to include the
> following:
> > >>
> > >> @provide_session
> > >> def refresh_from_db(self, session=3DNone, lock_for_update=3DFalse):
> > >>    """
> > >>    Refreshes the task instance from the database based on the primar=
y
> > key
> > >>    """
> > >>    TI =3D TaskInstance
> > >>
> > >>    if lock_for_update:
> > >>        ti =3D session.query(TI).filter(
> > >>            TI.dag_id =3D=3D self.dag_id,
> > >>            TI.task_id =3D=3D self.task_id,
> > >>            TI.execution_date =3D=3D self.execution_date,
> > >>        ).with_for_update().first()
> > >>    else:
> > >>        ti =3D session.query(TI).filter(
> > >>            TI.dag_id =3D=3D self.dag_id,
> > >>            TI.task_id =3D=3D self.task_id,
> > >>            TI.execution_date =3D=3D self.execution_date,
> > >>        ).first()
> > >>
> > >> And to use =E2=80=9Clock_for_update=3DTrue=E2=80=9D in TaskInstance.=
run. This locks the
> > >> record for update (with a FOR UPDATE) in the database (Postgres, Mys=
ql
> > >=3D
> > >> 5.7) and ensures a =E2=80=9Cselect=E2=80=9D needs to wait if it incl=
udes this record
> in
> > its
> > >> results. As soon as a =E2=80=9Csession.commit()=E2=80=9D occurs the =
lock is gone. The
> > >> window of for the race condition to occur is this much shorter now
> (the
> > >> time between a new scheduler run and the time it takes for the pytho=
n
> > >> interpreter to start =E2=80=9Cairflow run=E2=80=9D) though not entir=
ely closed.
> > >>
> > >> Due to the lock their *might* be a small performance hit, but it
> should
> > be
> > >> very small: only queries that include the aforementioned locked reco=
rd
> > will
> > >> be delayed for a few milliseconds. However, we might overlook
> something
> > so
> > >> I kindly request to review this PR, so we can make it part of 1.7.1.
> > >>
> > >> Thanks
> > >> Bolke
> > >>
> > >>
> > >>
> >
>

--001a113eae10a0d0f9053232c6ed--