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 2020/10/23 18:15:56 UTC
[GitHub] [airflow] kaxil opened a new pull request #11791: Use LocalExecutor by default with tmux + Breeze
kaxil opened a new pull request #11791:
URL: https://github.com/apache/airflow/pull/11791
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511062763
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
But still there is the lock in sqllite :(
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511064622
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
>But shoudl not it be the case when you enter breeze shell?
I am mainly thinking no for that, just so that users can replicate the stuff in CI locally when they enter just `./breeze`.
but when do they do `breeze start-airflow` it is for running some DAGs instead of just running unit-tests
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511081575
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
So far it never interfered. The default executor for Postgres/Mysql was Local (no variable set), for Sqlite it was Sequential (set by the variable in backend-sqlite.yml) . That's how it worked so far. somehow it looks like with the recent changes Sequential became the default ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #11791:
URL: https://github.com/apache/airflow/pull/11791
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511077557
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
I also think the right executor should be set if you just enter breeze with --backend postgres or --backend mysql to shell rather than use 'start-airflow' (new feature)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511079666
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
but won't that interfere when running tests using `pytest tests/some_file` inside `./breeze`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511092348
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
SequentialExecutor works with all the backends and have always worked with all backends, with SequentialExecutor tasks are just executed sequentially -- nothing more. Why do you think it would fail (except for the current bug)
That is the current behavior with Airflow too, the default executor is `SequentialExecutor` even if you used Postgres or MySQL until you change the executor separately too
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511086573
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
the instructions to run RCs and test alphas are more recent and were added after `start-airflow` command is might point. Our CI and breeze update more frequently (multiple times a week), so if it makes sense we should promote and tell users to use new stuff even if it is recent.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511085435
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
> i.e I feel there are 2 kind of uses currently I can think of
Not really. People were running breeze and then started airflow inside. 'start-airflow' is very recent addition and only there to automate tmux terminals and starting scheduler/webserver.
Most people simply start breeze and manually running webseerver/scheduler and get it working there.
I believe making airflow not work by default in breeze is a very bad idea.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511073978
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
we definitely need to fix sqlite as that is essential so for those users who run `tmux` from within `./breeze` it should still work. But we do recommend people to use `./breeze start-airflow` in docs too because it supports using an ENV file too and I feel it is very powerful because of that and coz it also support and init file
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511067882
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
So far what many people were doing when entering breeze they run tmux and then "airflow webserver" and "airlfow scheeduler" and they run dag testing. It would be surprising tot see a hanging run there without explanation.
"start-airflow" is rather new addition (2 weeks).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511080010
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
Also - the SequentialExecutor should not be default I think :( seems it is right now (but was not)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511081575
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
So far it never interfered. The default executor for Postgres/Mysql was Local, for Sqlite it was Sequential (set by the variable in backend-sqlite.yml) . That's how it worked so far. somehow it looks like with the recent changes Sequential became the default ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511077557
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
I also think the right executor should be set if you just enter breeze with --backend postgres or --backend mysql with shell rather than use 'start-airflow' (new feature)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511094942
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
apart from the current bug why do you think SequentialExecutor does not work with MySQL or Postgres
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511074527
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
>The CI tests run so far without having to set the executor - this is what I am really about it, so changing to a non-working executor by default, when it was working fine before is not a good idea.
Yeah, don't get me wrong, I am going to debug the sqlite issue too, this is just an optimization for people who just wants to test quickly with Postgres and MySQL -- will help in Alpha testing
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511093076
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
I really think we should simply detect when the combination of DB/executor is not going to work and fail hard. I think this is the root cause of the problem.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511080721
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
i.e I feel there are 2 kind of uses currently I can think of
1) People who run `./breeze --backend postgres` to run tests so that they can replicate their CI failures for examples
2) People who want to run DAGs and try out new features/ test RCs or alphas etc for whom `./breeze start-airflow --backend postgres` would be easier because it supports using an ENV file too and I feel it is very powerful because of that and coz it also supports init file, WDYT ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511086573
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
the instructions to run RCs and test alphas are more recent and were added after `start-airflow` command is my point. Our CI and breeze update more frequently (multiple times a week), so if it makes sense we should promote and tell users to use new stuff even if it is recent.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511063082
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
I don't want to do in docker-compose file as it will use that executor for running tests in CI too
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511074527
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
>The CI tests run so far without having to set the executor - this is what I am really about it, so changing to a non-working executor by default, when it was working fine before is not a good idea.
Yeah, don't get me wrong, I am going to debug the sqlite issue too, this is just an optimization for people who just wants to test
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511069787
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
The CI tests run so far without having to set the executor - this is what I am really about it, so changing to a non-working executor by default, when it was working fine before is not a good idea.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511084736
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
Are you sure about that, I can remember that is was always SequentialExecutor for me inside of breeze but maybe not.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511093076
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
I really thing we should simply detect when the combination of DB/executor is not going to work and fail hard. I think this is the root cause of the problem.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511062316
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
Probably better to add it in the docker-compose config
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511064750
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
And we should fail hard if executor == SEQUENTIAL + db is anything else than sqlite (as long as we fix sqlite)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511090379
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
that is correct, can you remember when it used to use `LocalExecutor` please before? I am honestly happy to do that and have approved your PR as long as you are sure it worked fine previously -- because I am not sure about that
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511089121
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
@potiuk you are confusing few things here. the bug with `SequentialExecutor` needs to be fixed as I said in my previous comments. This PR has nothing to do with fixing that bug, this is just an optimization like explained in PR description.
The bug has to be fixed separatly
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511089172
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
I am not sure if I am right about it - but it seems this is how it works now - we get sequential executor by default where we used to have local for Pg/Msql. Not sure where it came from, but it's just wrong default.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511060266
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
oh yes, ofcourse, let me add some conditionals
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
oh yes, ofcourse 🤦 , let me add some conditionals
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511063652
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
But shoudl not it be the case when you enter breeze shell?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511062027
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
I've added it here: https://github.com/apache/airflow/pull/11792
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511063771
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
>But still there is the lock in sqllite :(
Yeah we will need to find and fix for sqlite too, but this was mostly for users using `breeze start-airflow` for doing rc testing etc
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511076151
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
Added comment too @potiuk
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511088161
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
so far when you run:
```
./breeze
```
and then
```
tmux
```
and then
in one terminal - `airlfow webserver`
in another ``airlfow scheduler``
it just worked (because LocalExccutor was default for postgres/mysql).
seems that now default is the Sequential one - no matter what backend is used. Seems that if you do not set executor and the DB is postgres/mysql then the sequential one is used ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511090377
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
Or maybe sequential executor worked well with PG/MG and stopped working well with it... either way it's something we need to fix (at the least by failing hard if sequential is wrong for PG/MYSQL but best - changing the default)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511059455
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
I think it cannot be set here for SQLITE case
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511085817
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
>I believe making airflow not work by default in breeze is a very bad idea.
What do you mean? How will it make airflow not work
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511064095
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -21,6 +21,7 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ export AIRFLOW__CORE__EXECUTOR=${AIRFLOW__CORE__EXECUTOR:-LocalExecutor}
Review comment:
I think Local executor should be default if not set - this might be a better approach
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #11791: Use LocalExecutor by default with tmux + Breeze
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11791:
URL: https://github.com/apache/airflow/pull/11791#discussion_r511092348
##########
File path: scripts/in_container/run_tmux.sh
##########
@@ -22,6 +22,12 @@ if [[ ${START_AIRFLOW:="false"} == "true" ]]; then
export AIRFLOW__CORE__LOAD_DEFAULT_CONNECTIONS=${LOAD_DEFAULT_CONNECTIONS}
export AIRFLOW__CORE__LOAD_EXAMPLES=${LOAD_EXAMPLES}
+ # Use LocalExecutor if not set and if backend is not sqlite as it gives
Review comment:
SequentialExecutor works with all the backends and have always worked with all backends, with SequentialExecutor tasks are just executed sequentially -- nothing more. Why do you think it would fail (except for the current bug)
That is the current behavior with Airflow too (and has always been like that since its inception), the default executor is `SequentialExecutor` even if you used Postgres or MySQL until you change the executor separately too
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org