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/09/20 17:14:50 UTC
[GitHub] [airflow] potiuk opened a new pull request #18382: Production-level support for MSSQL
potiuk opened a new pull request #18382:
URL: https://github.com/apache/airflow/pull/18382
MSSQL has been somewhat experimental in the `main` branch, but as
we near releasing for 2.2.0 version, the image should support
the mssql at the level as it supports other databases.
This PR adds proper support for both PROD and CI images.
<!--
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/main/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/main/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.
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 pull request #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#issuecomment-923138727
Could MSSQL be used as a celery backend?
https://github.com/apache/airflow/blob/f76eaec65e834f9c8c3e4e0bec14bb63e76679b8/scripts/in_container/prod/entrypoint_prod.sh#L98-L106
--
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 #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#issuecomment-923493702
good comments - All solved @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.
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 #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#discussion_r712699139
##########
File path: docs/apache-airflow/howto/set-up-database.rst
##########
@@ -248,6 +248,13 @@ You can read more about transaction isolation and snapshot features at
GRANT ALL PRIVILEGES ON DATABASE airflow TO airflow_user;
+We recommend using the ``mssql+pyodbc`` driver and specifying it in your SqlAlchemy connection string.
+
+.. code-block:: text
+
+ mssql+pyodbc://<user>:<password>@<host>
Review comment:
On CI, we use a little more complex URI. This leads me to suppose that this example may be incomplete. https://github.com/apache/airflow/blob/main/scripts/ci/docker-compose/backend-mssql.yml#L23-L24
--
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 #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#issuecomment-923491522
> Only MSSQL does not include an example URI address for me to be able to. configure the database. SQLAlchemy supports multiple drivers. Which one is the best for production deployment?
@aneesh-joseph ?
--
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 #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#discussion_r712608347
##########
File path: Dockerfile.ci
##########
@@ -104,6 +104,7 @@ RUN mkdir -pv /usr/share/man/man1 \
COPY scripts/docker/*.sh /scripts/docker/
RUN bash /scripts/docker/install_mysql.sh dev \
+ && /scripts/docker/install_mssql.sh \
Review comment:
```suggestion
&& bash /scripts/docker/install_mssql.sh \
```
For consistency. See previous line.
--
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] ashb commented on pull request #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#issuecomment-924213763
How much does this add to the docker images size btw?
--
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 #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#discussion_r712608843
##########
File path: scripts/docker/install_mssql.sh
##########
@@ -0,0 +1,42 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+set -euo pipefail
+
+set -x
Review comment:
Why this script has so many ``set`` statements?
--
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 edited a comment on pull request #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#issuecomment-924410641
> How much does this add to the docker images size btw?
It increases the size of the image by 4MB - from 972 MB to 976 MB (~ 0.5%) See the discussion here: https://apache-airflow.slack.com/archives/CQAMHKWSJ/p1632131354042400
--
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 #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#discussion_r712898018
##########
File path: docs/apache-airflow/howto/set-up-database.rst
##########
@@ -248,6 +248,13 @@ You can read more about transaction isolation and snapshot features at
GRANT ALL PRIVILEGES ON DATABASE airflow TO airflow_user;
+We recommend using the ``mssql+pyodbc`` driver and specifying it in your SqlAlchemy connection string.
+
+.. code-block:: text
+
+ mssql+pyodbc://<user>:<password>@<host>
Review comment:
Should we add a database name to the example? I think it is a commonly used parameter. It needed to set up a database created in the paragraph above.
```
mssql+pyodbc://<user>:<password>@<host>
mssql+pyodbc://<user>:<password>@<host>/<database_name>
```
--
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 #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#issuecomment-923195364
> Could MSSQL be used as a celery backend?
Added support for the entrypoint . Good catch @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.
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 pull request #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#issuecomment-923611066
@potiuk SQLAlchemy published some recommendations about preferred drivers:
PyODBC has the best Unicode support.
https://docs.sqlalchemy.org/en/14/dialects/mssql.html#driver-unicode-support
> Deprecated since version 1.4: The mxODBC DBAPI is deprecated and will be removed in a future version. Please use one of the supported DBAPIs to connect to mssql.
https://docs.sqlalchemy.org/en/14/dialects/mssql.html#dialect-mssql-mxodbc-connect
>pymssql is currently not included in SQLAlchemy’s continuous integration (CI) testing.
https://docs.sqlalchemy.org/en/14/dialects/mssql.html#module-sqlalchemy.dialects.mssql.pymssql
--
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 #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#issuecomment-924410641
> How much does this add to the docker images size btw?
It increases the size of the image by 4MB - from 972 to 976. See the discussion here: https://apache-airflow.slack.com/archives/CQAMHKWSJ/p1632131354042400
--
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 #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#issuecomment-923612468
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] potiuk commented on a change in pull request #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#discussion_r712862192
##########
File path: docs/apache-airflow/howto/set-up-database.rst
##########
@@ -248,6 +248,13 @@ You can read more about transaction isolation and snapshot features at
GRANT ALL PRIVILEGES ON DATABASE airflow TO airflow_user;
+We recommend using the ``mssql+pyodbc`` driver and specifying it in your SqlAlchemy connection string.
+
+.. code-block:: text
+
+ mssql+pyodbc://<user>:<password>@<host>
Review comment:
This is the default one from the sqlalchemy docs:
https://docs.sqlalchemy.org/en/14/core/engines.html#microsoft-sql-server
We use mor complex one, but those are optional parameters, and I wouldn't dive into details of it (especially not before we have first users using 'for real' and making comments about it.
--
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 #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#issuecomment-923493565
> Only MSSQL does not include an example URI address for me to be able to. configure the database. SQLAlchemy supports multiple drivers. Which one is the best for production deployment?
I added mssql+pyodbc as this is the one a) we support in the the image b) we test during CI. Since this is rather fresh - I will leave it at that until we have an evidence that other drivers should be preferred (but then we should also change our tests and image to use 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.
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 #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#discussion_r712910772
##########
File path: docs/apache-airflow/howto/set-up-database.rst
##########
@@ -248,6 +248,13 @@ You can read more about transaction isolation and snapshot features at
GRANT ALL PRIVILEGES ON DATABASE airflow TO airflow_user;
+We recommend using the ``mssql+pyodbc`` driver and specifying it in your SqlAlchemy connection string.
+
+.. code-block:: text
+
+ mssql+pyodbc://<user>:<password>@<host>
Review comment:
I created a PR: https://github.com/apache/airflow/pull/18404
--
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 pull request #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#issuecomment-923486249
I am missing one more element. On [the database configuration](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/set-up-database.rst) page, we should add an example URL address.
For SQLite, we have the following fragment:
> An example URI for the sqlite database:
>
> sqlite:////home/airflow/airflow.db
For MySQL, we have the following fragment:
> We recommend using the mysqlclient driver and specifying it in your SqlAlchemy connection string.
>
> mysql+mysqldb://<user>:<password>@<host>[:<port>]/<dbname>
> But we also support the mysql-connector-python driver, which lets you connect through SSL without any cert options provided.
>
> mysql+mysqlconnector://<user>:<password>@<host>[:<port>]/<dbname>
For. PostgresSQL, we have the following fragment:
> We recommend using the psycopg2 driver and specifying it in your SqlAlchemy connection string.
>
> postgresql+psycopg2://<user>:<password>@<host>/<db>
Only MSSQL does not include an example URI address for me to be able to. configure the database. SQLAlchemy supports multiple drivers. Which one is the best for production deployment?
```
# pyodbc
mssql+pyodbc://scott:tiger@mydsn
# pymssql
mssql+pymssql://scott:tiger@hostname:port/dbname
# mxODBC
mssql+mxodbc://<username>:<password>@<dsnname>
```
--
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 merged pull request #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #18382:
URL: https://github.com/apache/airflow/pull/18382
--
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 #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#issuecomment-923191849
> Could MSSQL be used as a celery backend?
Very good point! Indeed I believe it can
--
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 #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#discussion_r712699139
##########
File path: docs/apache-airflow/howto/set-up-database.rst
##########
@@ -248,6 +248,13 @@ You can read more about transaction isolation and snapshot features at
GRANT ALL PRIVILEGES ON DATABASE airflow TO airflow_user;
+We recommend using the ``mssql+pyodbc`` driver and specifying it in your SqlAlchemy connection string.
+
+.. code-block:: text
+
+ mssql+pyodbc://<user>:<password>@<host>
Review comment:
On CI, we use a little more complex URI. This leads me to suppose that this example may be incomplete. https://github.com/apache/airflow/blob/c686241f4ceb62d52e9bfa607822e4b7a3c76222/scripts/ci/docker-compose/backend-mssql.yml#L23-L24
--
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 a change in pull request #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#discussion_r712862192
##########
File path: docs/apache-airflow/howto/set-up-database.rst
##########
@@ -248,6 +248,13 @@ You can read more about transaction isolation and snapshot features at
GRANT ALL PRIVILEGES ON DATABASE airflow TO airflow_user;
+We recommend using the ``mssql+pyodbc`` driver and specifying it in your SqlAlchemy connection string.
+
+.. code-block:: text
+
+ mssql+pyodbc://<user>:<password>@<host>
Review comment:
This is the default one from the sqlalchemy docs (I took it from there):
https://docs.sqlalchemy.org/en/14/core/engines.html#microsoft-sql-server
We use mor complex one, but those are optional parameters, and I wouldn't dive into details of it (especially not before we have first users using 'for real' and making comments about it.
--
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 a change in pull request #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#discussion_r712616198
##########
File path: Dockerfile.ci
##########
@@ -104,6 +104,7 @@ RUN mkdir -pv /usr/share/man/man1 \
COPY scripts/docker/*.sh /scripts/docker/
RUN bash /scripts/docker/install_mysql.sh dev \
+ && /scripts/docker/install_mssql.sh \
Review comment:
Right!
--
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 a change in pull request #18382: Production-level support for MSSQL
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #18382:
URL: https://github.com/apache/airflow/pull/18382#discussion_r712617119
##########
File path: scripts/docker/install_mssql.sh
##########
@@ -0,0 +1,42 @@
+#!/usr/bin/env bash
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+set -euo pipefail
+
+set -x
Review comment:
Good point - no idea. Same in mysql - remnant of an old refactor which was missed I guessed.
--
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