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/05/19 15:30:43 UTC
[GitHub] [airflow] Junnplus opened a new pull request #15945: Extract database options to new section
Junnplus opened a new pull request #15945:
URL: https://github.com/apache/airflow/pull/15945
closes: https://github.com/apache/airflow/issues/15930
The following configurations have been moved from `[core]` to the new `[database]` section.
- `sql_alchemy_conn`
- `sql_engine_encoding`
- `sql_engine_collation_for_ids`
- `sql_alchemy_pool_enabled`
- `sql_alchemy_pool_size`
- `sql_alchemy_max_overflow`
- `sql_alchemy_pool_recycle`
- `sql_alchemy_pool_pre_ping`
- `sql_alchemy_schema`
- `sql_alchemy_connect_args`
- `load_default_connections`
- `max_db_retries`
---
**^ 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] mik-laj commented on pull request #15945: Extract database options to new section
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #15945:
URL: https://github.com/apache/airflow/pull/15945#issuecomment-880487995
@Junnplus Do you need any help with this contribution?
--
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] mik-laj commented on a change in pull request #15945: Extract database options to new section
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15945:
URL: https://github.com/apache/airflow/pull/15945#discussion_r641948984
##########
File path: UPDATING.md
##########
@@ -84,6 +84,23 @@ Previously this was controlled by `non_pooled_task_slot_count` in `[core]` secti
## Airflow 2.1.0
+### Database configuration has been moved to new section
+
+The following configurations have been moved from `[core]` to the new `[database]` section.
+
+- `sql_alchemy_conn`
Review comment:
renaming an option and renaming a section is a breaking change, so we should try to limit the number of change and combine them.
--
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] ashb commented on a change in pull request #15945: Extract database options to new section
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15945:
URL: https://github.com/apache/airflow/pull/15945#discussion_r671403754
##########
File path: UPDATING.md
##########
@@ -84,6 +84,23 @@ Previously this was controlled by `non_pooled_task_slot_count` in `[core]` secti
## Airflow 2.1.0
+### Database configuration has been moved to new section
+
+The following configurations have been moved from `[core]` to the new `[database]` section.
+
+- `sql_alchemy_conn`
Review comment:
It doesn't have to be breaking -- there is already the mechanism in our ConfigParser subclass for moving/renaming configs.
--
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] Junnplus closed pull request #15945: Extract database options to new section
Posted by GitBox <gi...@apache.org>.
Junnplus closed pull request #15945:
URL: https://github.com/apache/airflow/pull/15945
--
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] Junnplus commented on pull request #15945: Extract database options to new section
Posted by GitBox <gi...@apache.org>.
Junnplus commented on pull request #15945:
URL: https://github.com/apache/airflow/pull/15945#issuecomment-890368746
@mik-laj I'm sorry for taking so long to reply, I will close this PR because i dont't have time to work this.
--
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] mik-laj commented on a change in pull request #15945: Extract database options to new section
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15945:
URL: https://github.com/apache/airflow/pull/15945#discussion_r636983257
##########
File path: airflow/config_templates/config.yml
##########
@@ -436,6 +322,124 @@
example: ~
default: ""
+- name: database
+ description: ~
+ options:
+ - name: sql_alchemy_conn
+ description: |
+ The SqlAlchemy connection string to the metadata database.
+ SqlAlchemy supports many different database engines.
+ More information here:
+ http://airflow.apache.org/docs/apache-airflow/stable/howto/set-up-database.html#database-uri
+ version_added: ~
Review comment:
```suggestion
version_added: 2.2
```
In other options, we should set the same value.
--
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] Junnplus commented on a change in pull request #15945: Extract database options to new section
Posted by GitBox <gi...@apache.org>.
Junnplus commented on a change in pull request #15945:
URL: https://github.com/apache/airflow/pull/15945#discussion_r641946019
##########
File path: UPDATING.md
##########
@@ -84,6 +84,23 @@ Previously this was controlled by `non_pooled_task_slot_count` in `[core]` secti
## Airflow 2.1.0
+### Database configuration has been moved to new section
+
+The following configurations have been moved from `[core]` to the new `[database]` section.
Review comment:
I like shorter env vars, but I hope to keep the section name as `database`. cc @mik-laj
--
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] mik-laj commented on a change in pull request #15945: Extract database options to new section
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15945:
URL: https://github.com/apache/airflow/pull/15945#discussion_r641948984
##########
File path: UPDATING.md
##########
@@ -84,6 +84,23 @@ Previously this was controlled by `non_pooled_task_slot_count` in `[core]` secti
## Airflow 2.1.0
+### Database configuration has been moved to new section
+
+The following configurations have been moved from `[core]` to the new `[database]` section.
+
+- `sql_alchemy_conn`
Review comment:
Renaming an option and renaming a section is a breaking changes, so we should try to limit the number of this type of change and combine them.
--
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] Junnplus commented on a change in pull request #15945: Extract database options to new section
Posted by GitBox <gi...@apache.org>.
Junnplus commented on a change in pull request #15945:
URL: https://github.com/apache/airflow/pull/15945#discussion_r637411772
##########
File path: airflow/kubernetes/pod_template_file_examples/dags_in_image_template.yaml
##########
@@ -36,7 +36,7 @@ spec:
secretKeyRef:
name: RELEASE-NAME-fernet-key
key: fernet-key
- - name: AIRFLOW__CORE__SQL_ALCHEMY_CONN
Review comment:
Isn't this just an example? or keep old env variable make user friendly ?
--
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] mik-laj commented on a change in pull request #15945: Extract database options to new section
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15945:
URL: https://github.com/apache/airflow/pull/15945#discussion_r641946597
##########
File path: UPDATING.md
##########
@@ -84,6 +84,23 @@ Previously this was controlled by `non_pooled_task_slot_count` in `[core]` secti
## Airflow 2.1.0
+### Database configuration has been moved to new section
+
+The following configurations have been moved from `[core]` to the new `[database]` section.
Review comment:
I have no preferences. both names look good to me.
--
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] ashb commented on a change in pull request #15945: Extract database options to new section
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #15945:
URL: https://github.com/apache/airflow/pull/15945#discussion_r637550804
##########
File path: UPDATING.md
##########
@@ -84,6 +84,23 @@ Previously this was controlled by `non_pooled_task_slot_count` in `[core]` secti
## Airflow 2.1.0
+### Database configuration has been moved to new section
+
+The following configurations have been moved from `[core]` to the new `[database]` section.
Review comment:
How about db for shorter env vars? `AIRFLOW__DB_*`
##########
File path: UPDATING.md
##########
@@ -84,6 +84,23 @@ Previously this was controlled by `non_pooled_task_slot_count` in `[core]` secti
## Airflow 2.1.0
+### Database configuration has been moved to new section
+
+The following configurations have been moved from `[core]` to the new `[database]` section.
+
+- `sql_alchemy_conn`
+- `sql_engine_encoding`
+- `sql_engine_collation_for_ids`
+- `sql_alchemy_pool_enabled`
+- `sql_alchemy_pool_size`
+- `sql_alchemy_max_overflow`
+- `sql_alchemy_pool_recycle`
+- `sql_alchemy_pool_pre_ping`
+- `sql_alchemy_schema`
+- `sql_alchemy_connect_args`
Review comment:
Ditto here, if they are in a dedicated section then `sql`/`sql_alchemy` prefix is largely redundant now
##########
File path: UPDATING.md
##########
@@ -84,6 +84,23 @@ Previously this was controlled by `non_pooled_task_slot_count` in `[core]` secti
## Airflow 2.1.0
+### Database configuration has been moved to new section
+
+The following configurations have been moved from `[core]` to the new `[database]` section.
+
+- `sql_alchemy_conn`
Review comment:
If we are renaming this lets go all the way, and just make this uri? `AIRFLOW__DB__URI`
--
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] mik-laj commented on a change in pull request #15945: Extract database options to new section
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15945:
URL: https://github.com/apache/airflow/pull/15945#discussion_r637630089
##########
File path: airflow/kubernetes/pod_template_file_examples/dags_in_image_template.yaml
##########
@@ -36,7 +36,7 @@ spec:
secretKeyRef:
name: RELEASE-NAME-fernet-key
key: fernet-key
- - name: AIRFLOW__CORE__SQL_ALCHEMY_CONN
Review comment:
Oh yes. This file is an example only, but pod_template_file in /chart/ directory is used by Helm Chart.
--
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] github-actions[bot] commented on pull request #15945: Extract database options to new section
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15945:
URL: https://github.com/apache/airflow/pull/15945#issuecomment-844239007
--
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] Junnplus commented on a change in pull request #15945: Extract database options to new section
Posted by GitBox <gi...@apache.org>.
Junnplus commented on a change in pull request #15945:
URL: https://github.com/apache/airflow/pull/15945#discussion_r641946612
##########
File path: UPDATING.md
##########
@@ -84,6 +84,23 @@ Previously this was controlled by `non_pooled_task_slot_count` in `[core]` secti
## Airflow 2.1.0
+### Database configuration has been moved to new section
+
+The following configurations have been moved from `[core]` to the new `[database]` section.
+
+- `sql_alchemy_conn`
Review comment:
This PR is to extract the database related options, keeping the original option name I think is necessary .
--
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] mik-laj commented on a change in pull request #15945: Extract database options to new section
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15945:
URL: https://github.com/apache/airflow/pull/15945#discussion_r636985154
##########
File path: airflow/kubernetes/pod_template_file_examples/dags_in_image_template.yaml
##########
@@ -36,7 +36,7 @@ spec:
secretKeyRef:
name: RELEASE-NAME-fernet-key
key: fernet-key
- - name: AIRFLOW__CORE__SQL_ALCHEMY_CONN
Review comment:
We should also keep the old envvariaable to maintain compatibility with Airflow 1.10. Helm Chart is compatible with Airflow 1.10 and 2.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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #15945: Extract database options to new section
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15945:
URL: https://github.com/apache/airflow/pull/15945#discussion_r636986700
##########
File path: scripts/ci/docker-compose/backend-mysql.yml
##########
@@ -20,8 +20,8 @@ services:
airflow:
environment:
- BACKEND=mysql
- - AIRFLOW__CORE__SQL_ALCHEMY_CONN=mysql://root@mysql/airflow?charset=utf8mb4
- - AIRFLOW__CORE__SQL_ENGINE_COLLATION_FOR_IDS=utf8mb3_general_ci
+ - AIRFLOW__DATABASE__SQL_ALCHEMY_CONN=mysql://root@mysql/airflow?charset=utf8mb4
Review comment:
We should also keep the old variable so that we can test the 1.10 releases.
--
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] mik-laj commented on a change in pull request #15945: Extract database options to new section
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #15945:
URL: https://github.com/apache/airflow/pull/15945#discussion_r636987661
##########
File path: scripts/in_container/prod/entrypoint_prod.sh
##########
@@ -201,11 +201,11 @@ function wait_for_airflow_db() {
# then uses netcat to check that the host is reachable.
# This is only used by Airflow 1.10+ as there are no built-in commands to check the db connection.
local connection_url
- if [[ -n "${AIRFLOW__CORE__SQL_ALCHEMY_CONN_CMD=}" ]]; then
- connection_url="$(eval "${AIRFLOW__CORE__SQL_ALCHEMY_CONN_CMD}")"
+ if [[ -n "${AIRFLOW__DATABASE__SQL_ALCHEMY_CONN_CMD=}" ]]; then
Review comment:
We here should support the old value as well to keep Airflow 1.10 compatible
--
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