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/13 18:02:51 UTC
[GitHub] [airflow] Kaash opened a new pull request #11512: Resolve MSSQL DAG serialization bug by changing datatype for execution date
Kaash opened a new pull request #11512:
URL: https://github.com/apache/airflow/pull/11512
Submitting a PR to resolve the below reported issue when enabling serialization with MSSQ:: https://github.com/apache/airflow/issues/11493
----------------------------------------------------------------
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 edited a comment on pull request #11512: Resolve MSSQL DAG serialization bug by changing datatype for execution date
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #11512:
URL: https://github.com/apache/airflow/pull/11512#issuecomment-707941866
@alexmc-ms @aneesh-joseph @jmfreeman @mattinbits @jatskin Can you look at it? I don't have experience with MSSQL.
----------------------------------------------------------------
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 #11512: Resolve MSSQL DAG serialization bug by changing datatype for execution date
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11512:
URL: https://github.com/apache/airflow/pull/11512#discussion_r505184777
##########
File path: airflow/migrations/versions/52d53670a240_fix_mssql_exec_date_rendered_task_instance.py
##########
@@ -0,0 +1,74 @@
+#
+# 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.
+
+"""fix_mssql_exec_date_rendered_task_instance_fields_for_MSSQL
+
+Revision ID: 52d53670a240
+Revises: 98271e7606e2
+Create Date: 2020-10-13 15:13:24.911486
+
+"""
+import sqlalchemy as sa
+from sqlalchemy.dialects import mssql
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = '52d53670a240'
+down_revision = '98271e7606e2'
+branch_labels = None
+depends_on = None
+
+TABLE_NAME = 'rendered_task_instance_fields'
+
+
+def upgrade():
+ """
+ Recreate RenderedTaskInstanceFields table changing timestamp to datetime2(6) when using MSSQL as backend
+ """
+ conn = op.get_bind()
+ if conn.dialect.name == "mssql":
+ json_type = sa.Text
+ op.drop_table(TABLE_NAME) # pylint: disable=no-member
Review comment:
Wat.
Cool, thanks
----------------------------------------------------------------
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 #11512: Resolve MSSQL DAG serialization bug by changing datatype for execution date
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11512:
URL: https://github.com/apache/airflow/pull/11512#issuecomment-707943992
[The Workflow run](https://github.com/apache/airflow/actions/runs/304935460) is cancelling this PR. Building image for the PR has been cancelled
----------------------------------------------------------------
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] Kaash commented on a change in pull request #11512: Resolve MSSQL DAG serialization bug by changing datatype for execution date
Posted by GitBox <gi...@apache.org>.
Kaash commented on a change in pull request #11512:
URL: https://github.com/apache/airflow/pull/11512#discussion_r504939704
##########
File path: airflow/migrations/versions/52d53670a240_fix_mssql_exec_date_rendered_task_instance.py
##########
@@ -0,0 +1,74 @@
+#
+# 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.
+
+"""fix_mssql_exec_date_rendered_task_instance_fields_for_MSSQL
+
+Revision ID: 52d53670a240
+Revises: 98271e7606e2
+Create Date: 2020-10-13 15:13:24.911486
+
+"""
+import sqlalchemy as sa
+from sqlalchemy.dialects import mssql
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = '52d53670a240'
+down_revision = '98271e7606e2'
+branch_labels = None
+depends_on = None
+
+TABLE_NAME = 'rendered_task_instance_fields'
+
+
+def upgrade():
+ """
+ Recreate RenderedTaskInstanceFields table changing timestamp to datetime2(6) when using MSSQL as backend
+ """
+ conn = op.get_bind()
+ if conn.dialect.name == "mssql":
+ json_type = sa.Text
+ op.drop_table(TABLE_NAME) # pylint: disable=no-member
Review comment:
We cannot alter column 'execution_date' because it is 'timestamp'. We're basically stuck with what we have. Bear in mind that the way this is at the moment (sa.TIMESTAMP) doesn't work with MSSQL
----------------------------------------------------------------
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] boring-cyborg[bot] commented on pull request #11512: Resolve MSSQL DAG serialization bug by changing datatype for execution date
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #11512:
URL: https://github.com/apache/airflow/pull/11512#issuecomment-709286363
Awesome work, congrats on your first merged pull request!
----------------------------------------------------------------
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 #11512: Resolve MSSQL DAG serialization bug by changing datatype for execution date
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #11512:
URL: https://github.com/apache/airflow/pull/11512#discussion_r504539441
##########
File path: airflow/migrations/versions/52d53670a240_fix_mssql_exec_date_rendered_task_instance.py
##########
@@ -0,0 +1,74 @@
+#
+# 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.
+
+"""fix_mssql_exec_date_rendered_task_instance_fields_for_MSSQL
+
+Revision ID: 52d53670a240
+Revises: 98271e7606e2
+Create Date: 2020-10-13 15:13:24.911486
+
+"""
+import sqlalchemy as sa
+from sqlalchemy.dialects import mssql
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = '52d53670a240'
+down_revision = '98271e7606e2'
+branch_labels = None
+depends_on = None
+
+TABLE_NAME = 'rendered_task_instance_fields'
+
+
+def upgrade():
+ """
+ Recreate RenderedTaskInstanceFields table changing timestamp to datetime2(6) when using MSSQL as backend
+ """
+ conn = op.get_bind()
+ if conn.dialect.name == "mssql":
+ json_type = sa.Text
+ op.drop_table(TABLE_NAME) # pylint: disable=no-member
Review comment:
Do we really have to drop the whole table, instead of just changing the column type?
Can you link (here, not in code) the difference between DATETIME2 and what ever `sa.TIMESTAMP` gives?
----------------------------------------------------------------
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] boring-cyborg[bot] commented on pull request #11512: Resolve MSSQL DAG serialization bug by changing datatype for execution date
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #11512:
URL: https://github.com/apache/airflow/pull/11512#issuecomment-707914276
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, pylint and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/master/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/master/docs/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/master/BREEZE.rst) for testing locally, itโs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better ๐.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
----------------------------------------------------------------
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 merged pull request #11512: Resolve MSSQL DAG serialization bug by changing datatype for execution date
Posted by GitBox <gi...@apache.org>.
ashb merged pull request #11512:
URL: https://github.com/apache/airflow/pull/11512
----------------------------------------------------------------
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 #11512: Resolve MSSQL DAG serialization bug by changing datatype for execution date
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #11512:
URL: https://github.com/apache/airflow/pull/11512#issuecomment-707941866
@alexmc-ms @aneesh-joseph @jmfreeman @mattinbits @jatskin Can you look at 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #11512: Resolve MSSQL DAG serialization bug by changing datatype for execution date
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11512:
URL: https://github.com/apache/airflow/pull/11512#issuecomment-707944490
Hey @Kaash . Can you please rebase this one to latest master. We fixed (hopefully) a problem with queues of jobs for GitHub actions and I think when you rebase, it shoudl run much faster (more info on devlist 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Kaash commented on a change in pull request #11512: Resolve MSSQL DAG serialization bug by changing datatype for execution date
Posted by GitBox <gi...@apache.org>.
Kaash commented on a change in pull request #11512:
URL: https://github.com/apache/airflow/pull/11512#discussion_r504947282
##########
File path: airflow/migrations/versions/52d53670a240_fix_mssql_exec_date_rendered_task_instance.py
##########
@@ -0,0 +1,74 @@
+#
+# 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.
+
+"""fix_mssql_exec_date_rendered_task_instance_fields_for_MSSQL
+
+Revision ID: 52d53670a240
+Revises: 98271e7606e2
+Create Date: 2020-10-13 15:13:24.911486
+
+"""
+import sqlalchemy as sa
+from sqlalchemy.dialects import mssql
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = '52d53670a240'
+down_revision = '98271e7606e2'
+branch_labels = None
+depends_on = None
+
+TABLE_NAME = 'rendered_task_instance_fields'
+
+
+def upgrade():
+ """
+ Recreate RenderedTaskInstanceFields table changing timestamp to datetime2(6) when using MSSQL as backend
+ """
+ conn = op.get_bind()
+ if conn.dialect.name == "mssql":
+ json_type = sa.Text
+ op.drop_table(TABLE_NAME) # pylint: disable=no-member
Review comment:
Also, I should add that this is because SQL Server doesn't allow it. Below is the official explanation from Microsoft documentation - hope this helps.
https://docs.microsoft.com/en-us/sql/t-sql/statements/alter-table-transact-sql?view=sql-server-ver15:
_****ALTER COLUMN
Specifies that the named column is to be changed or altered.
The modified column can't be:
A column with a timestamp data type.****_
----------------------------------------------------------------
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 #11512: Resolve MSSQL DAG serialization bug by changing datatype for execution date
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11512:
URL: https://github.com/apache/airflow/pull/11512#issuecomment-708676379
[The Workflow run](https://github.com/apache/airflow/actions/runs/307373711) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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