You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/08/11 23:15:29 UTC
[GitHub] [airflow] andrewgodwin opened a new pull request #17565: Add Triggerer warning banner in UI
andrewgodwin opened a new pull request #17565:
URL: https://github.com/apache/airflow/pull/17565
This adds a "triggerer is not running" banner in the Web UI, similar to the one that shows when the scheduler is not running.
As well as checking for a TriggererJob, though, it also checks to make sure there are pending triggers - this ensures Airflow users who do not use Deferred Operators are not affected.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] andrewgodwin commented on a change in pull request #17565: Add Triggerer warning banner in UI
Posted by GitBox <gi...@apache.org>.
andrewgodwin commented on a change in pull request #17565:
URL: https://github.com/apache/airflow/pull/17565#discussion_r687997977
##########
File path: airflow/jobs/triggerer_job.py
##########
@@ -66,6 +67,15 @@ def register_signals(self) -> None:
signal.signal(signal.SIGINT, self._exit_gracefully)
signal.signal(signal.SIGTERM, self._exit_gracefully)
+ @provide_session
+ def is_needed(self, session) -> bool:
+ """
+ Tests if the triggerer job needs to be run (i.e., if there are triggers
+ in the trigger table).
+ This is used for the warning boxes in the UI.
+ """
+ return session.query(Trigger).count() > 0
Review comment:
Right - I remember seeing this somewhere else, it makes SQLAlchemy emit an actually-efficient query?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #17565: Add Triggerer warning banner in UI
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #17565:
URL: https://github.com/apache/airflow/pull/17565#discussion_r687991180
##########
File path: airflow/jobs/triggerer_job.py
##########
@@ -66,6 +67,15 @@ def register_signals(self) -> None:
signal.signal(signal.SIGINT, self._exit_gracefully)
signal.signal(signal.SIGTERM, self._exit_gracefully)
+ @provide_session
+ def is_needed(self, session) -> bool:
+ """
+ Tests if the triggerer job needs to be run (i.e., if there are triggers
+ in the trigger table).
+ This is used for the warning boxes in the UI.
+ """
+ return session.query(Trigger).count() > 0
Review comment:
```suggestion
return session.query(func.count(Trigger.id)).scalar() > 0
```
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #17565: Add Triggerer warning banner in UI
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17565:
URL: https://github.com/apache/airflow/pull/17565#issuecomment-897916038
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #17565: Add Triggerer warning banner in UI
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #17565:
URL: https://github.com/apache/airflow/pull/17565#discussion_r688007747
##########
File path: airflow/jobs/triggerer_job.py
##########
@@ -66,6 +67,15 @@ def register_signals(self) -> None:
signal.signal(signal.SIGINT, self._exit_gracefully)
signal.signal(signal.SIGTERM, self._exit_gracefully)
+ @provide_session
+ def is_needed(self, session) -> bool:
+ """
+ Tests if the triggerer job needs to be run (i.e., if there are triggers
+ in the trigger table).
+ This is used for the warning boxes in the UI.
+ """
+ return session.query(Trigger).count() > 0
Review comment:
yeah
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil edited a comment on pull request #17565: Add Triggerer warning banner in UI
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #17565:
URL: https://github.com/apache/airflow/pull/17565#issuecomment-898057148
> I guess "Triggerer" is now mandatory component, that should always be running?
>
> If that's the case we should also add "triggerer" as another tmux screen in `start-airflow` breeze command
Yup, we are going to follow up by including it in Helm Chart, Breeze and docker-compose files in a separate PR
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #17565: Add Triggerer warning banner in UI
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17565:
URL: https://github.com/apache/airflow/pull/17565#issuecomment-898056168
I guess "Triggerer" is now mandatory component, that should always be running?
If that's the case we should also add "triggerer" as another tmux screen in `start-airflow` breeze command
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] andrewgodwin commented on pull request #17565: Add Triggerer warning banner in UI
Posted by GitBox <gi...@apache.org>.
andrewgodwin commented on pull request #17565:
URL: https://github.com/apache/airflow/pull/17565#issuecomment-898073795
It is not _mandatory_ - you only need to run it if you plan to use Deferrable Operators. That's why this PR only shows the warning if it's not running _and_ you have triggers waiting to execute; if you just want to upgrade to 2.2 and not use the deferrable feature, no work is required.
That said, yes, we're planning to add it into Helm/compose/etc. shortly.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil merged pull request #17565: Add Triggerer warning banner in UI
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #17565:
URL: https://github.com/apache/airflow/pull/17565
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #17565: Add Triggerer warning banner in UI
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #17565:
URL: https://github.com/apache/airflow/pull/17565#issuecomment-898057148
> I guess "Triggerer" is now mandatory component, that should always be running?
>
> If that's the case we should also add "triggerer" as another tmux screen in `start-airflow` breeze command
Yup, we are going to follow-up by including it on Helm Chart, Breeze and docker-compose files in a separate PR
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org