Improve compatibility with mssql #9973

aneesh-joseph · 2020-07-24T13:06:39Z

Improve compatibility with MSSQL
Make it easier to test against mssql locally ( ./breeze --backend=mssql )
Add MSSQL based tests to Airflow's CI

^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

kaxil

As discussed in #9926 I would like us to improve the compatibility but won't like to add more test to our existing already large CI runs.

@potiuk is going to carry out a survey and then start the discussion on mailing list about it. Just a couple of days back, the large number of running test on v1-10-test causes a long-waiting queue.

kaxil

Whoops I see you did not add tests, just breeze configs which allows you to run tests on your machine. I am completely fine with that 👍

Dockerfile.ci

potiuk

LGTM. But I think we should add somewhere (in README.md) next to supported versions, that mssql support is experimental and that we are evaluating it.

Dockerfile.ci

aneesh-joseph · 2020-07-24T17:49:44Z

doesn't new pushes to the branch cancel previous workflow runs? - looks like the CI workflows are running to completion for every push I made to this branch and the previous ones are not getting cancelled out. Is it something to do with the ci.yml in this branch?

potiuk · 2020-07-24T17:51:05Z

doesn't new pushes to the branch cancel previous workflow runs? - looks like the CI workflows are running to completion for every push I made to this branch and the previous ones are not getting cancelled out. Is it something to do with the ci.yml in this branch?

They should cancel it. Let me see.

potiuk · 2020-07-24T17:54:05Z

Ah... It seems that PRs run from the fork do not have access to cancel the already running PRs :( (see the last error message)... Hmm that makes the whole cancel approach we have only work for master merges and direct pushes to v1-10-test branch :(.

Maybe we will have to figure out something else. I guess you cannot cancel running PR of yours @aneesh-joseph ? Only committers can do i t I believe?

{
  eventName: 'pull_request',
  sha: 'f5cc73b67463ce2c21e74f7a11d13bc98d08f2e0',
  headSha: '09dff0cd3a4acac259f21785b604ce41c28f70a1',
  branch: 'improve-mssql-compat',
  owner: 'apache',
  repo: 'airflow'
}
Found input: 1029499
Found token: yes
Found 5 runs total.
Found 2 runs in progress.
Cancelling another run:  {
  id: 181255422,
  head_sha: '93f198dbd05927ae3e855f95cd13af5a196be190',
  status: 'queued'
}
Error while cancelling workflow_id 1029499: Resource not accessible by integration
Done.

aneesh-joseph · 2020-07-24T17:57:47Z

Ah... It seems that PRs run from the fork do not have access to cancel the already running PRs :( (see the last error message)... Hmm that makes the whole cancel approach we have only work for master merges and direct pushes to v1-10-test branch :(.

Maybe we will have to figure out something else. I guess you cannot cancel running PR of yours @aneesh-joseph ? Only committers can do i t I believe?
{
  eventName: 'pull_request',
  sha: 'f5cc73b67463ce2c21e74f7a11d13bc98d08f2e0',
  headSha: '09dff0cd3a4acac259f21785b604ce41c28f70a1',
  branch: 'improve-mssql-compat',
  owner: 'apache',
  repo: 'airflow'
}
Found input: 1029499
Found token: yes
Found 5 runs total.
Found 2 runs in progress.
Cancelling another run:  {
  id: 181255422,
  head_sha: '93f198dbd05927ae3e855f95cd13af5a196be190',
  status: 'queued'
}
Error while cancelling workflow_id 1029499: Resource not accessible by integration
Done.

oops, and yes don't seem to have access to cancel runs from my PR

potiuk · 2020-07-24T18:00:33Z

oops, and yes don't seem to have access to cancel runs from my PR

That's the root of the problem - the running PRs have only read-access token to apache/airflow repository :(. Not sure if we will be able to do much about it

kaxil · 2020-07-24T18:01:29Z

oops, and yes don't seem to have access to cancel runs from my PR

That's the root of the problem - the running PRs have only read-access token to apache/airflow repository :(. Not sure if we will be able to do much about it

That issue has caused all sorts of issues with Github Action in general :(

potiuk · 2020-07-24T18:04:19Z

That issue has caused all sorts of issues with Github Action in general :(

Yeah - but that's for a very good reason ... We do not want random fork's PR to make any changes to our repo before it gets merged. But I think eventually some of those permissions (in this case we need "cancel any PR from the same fork and only from the same fork" permission) will be added to GA.

kaxil · 2020-07-24T18:08:31Z

That issue has caused all sorts of issues with Github Action in general :(

Yeah - but that's for a very good reason ... We do not want random fork's PR to make any changes to our repo before it gets merged. But I think eventually some of those permissions (in this case we need "cancel any PR from the same fork and only from the same fork" permission) will be added to GA.

The permission should have been configurable I feel similar to how we did with the bot. The number of people asking for this feature has increased significantly - hopefully Github now updates that :)

feluelle

LGTM, but I am wondering how much performance difference there is after changing the models code regarding the sql alchemy queries you had to modify.

If that is not necessary to modify, can/should we revert it? If not this code should be in sql alchemy mssql?

jarkkorantala · 2020-07-28T09:43:25Z

LGTM, but I am wondering how much performance difference there is after changing the models code regarding the sql alchemy queries you had to modify.

The change on airflow/models/dag.py is necessary, as it's moving the rendering decision from a hard-coded Postgres syntax is_paused IS True to a variable dialect syntax picked by SQLAlchemy (eg. is_paused IS True for Postgres, is_paused = 1 for SQL Server).

The changes on airflow/models/dagcode.py and airflow/models/serialized_dag.py look like they could incur a performance penalty as the query optimizer is not aware that only the first result will be read. Does SQL Server not support the original exists query?

aneesh-joseph · 2020-07-28T11:04:05Z

LGTM, but I am wondering how much performance difference there is after changing the models code regarding the sql alchemy queries you had to modify.

The change on airflow/models/dag.py is necessary, as it's moving the rendering decision from a hard-coded Postgres syntax is_paused IS True to a variable dialect syntax picked by SQLAlchemy (eg. is_paused IS True for Postgres, is_paused = 1 for SQL Server).

The changes on airflow/models/dagcode.py and airflow/models/serialized_dag.py look like they could incur a performance penalty as the query optimizer is not aware that only the first result will be read. Does SQL Server not support the original exists query?

it does support exists, but doesn't support EXISTS expression in the columns clause of a SELECT.

Failing airflow tests with mssql and exists checks - https://github.com/aneesh-joseph/airflow-tests/pull/15/checks?check_run_id=918457634

so I had to do the change in this PR or will have to change it to something like:

from sqlalchemy.sql.expression import literal
return  session.query(literal(True)).filter(
    session.query(cls)
    .filter(cls.dag_id == dag_id)
    .exists()
).scalar()

jarkkorantala · 2020-07-28T12:08:09Z

from sqlalchemy.sql.expression import literal
return  session.query(literal(True)).filter(
    session.query(cls)
    .filter(cls.dag_id == dag_id)
    .exists()
).scalar()

This looks like a better solution than the one with .first(), but it still finds reads through all of the matching values.
Adding a limit should fix that:

from sqlalchemy import true
return (
    session.query(true())
    .filter(
        session.query(cls).filter(cls.dag_id == dag_id).exists()
    )
    .limit(1)
    .scalar()
)

Maybe scalar already does that though?
Update: no, scalar doesn't add limits.

Update: first() does add limits though, so the version in the PR is already close to optimal. it does however deserialize the entire object, so could be improved with something like

from sqlalchemy import select, true
return (
    session.execute(
        select([true()])
        .where(cls.dag_id == dag_id)
        .limit(1)
    ).scalar()
)

aneesh-joseph · 2020-07-29T07:41:09Z

This looks like a better solution than the one with .first()

@jarkkorantala yes, but it breaks on My SQL - sqlalchemy/sqlalchemy#5481

it does however deserialize the entire object

I have updated the PR so that it queries for literal(True) instead of querying the complete object.

These checks are now getting translated into the below queries

MySQL

SELECT 1 AS param_1
FROM dag_code
WHERE dag_code.fileloc_hash = 15056821532142474
LIMIT 1

MS SQL

SELECT TOP 1 1 AS param_1
FROM dag_code
WHERE dag_code.fileloc_hash = 15056821532142474

SQLite

SELECT 1 AS param_1
FROM dag_code
WHERE dag_code.fileloc_hash = 15056821532142474
LIMIT 1 OFFSET 0

Postgres

SELECT True AS param_1
FROM dag_code
WHERE dag_code.fileloc_hash = 15056821532142474
LIMIT 1

Before this PR, they were getting translated into

MySQL

SELECT EXISTS (SELECT *
FROM dag_code
WHERE dag_code.fileloc_hash = %s) AS anon_1

MS SQL(wrong query)

SELECT EXISTS (SELECT *
FROM dag_code
WHERE dag_code.fileloc_hash = 15056821532142474) AS anon_1

SQLite

SELECT EXISTS (SELECT *
FROM dag_code
WHERE dag_code.fileloc_hash = 15056821532142474) AS anon_1

Postgres

SELECT EXISTS (SELECT *
FROM dag_code
WHERE dag_code.fileloc_hash = 15056821532142474) AS anon_1

aneesh-joseph · 2021-05-26T03:02:41Z

@potiuk thank you, that sorted it.. btw does the master build run on self hosted runners which have more memory.. should we enable mssql integration tests for master builds?

potiuk · 2021-05-26T09:24:35Z

@potiuk thank you, that sorted it.. btw does the master build run on self hosted runners which have more memory.. should we enable mssql integration tests for master builds?

It does and it is already sorted out :). the change I added is in the branch that is only executed when memory on the instance is less than < 32 G or so - this means that integration tests will run on master in MSSQL as well :).

if [[ ${test_types_to_run} == *"Integration"* ]]; then
        if (( MEMORY_AVAILABLE_FOR_DOCKER < MEMORY_REQUIRED_FOR_INTEGRATION_TEST_PARALLEL_RUN )) ; then
            # In case of Integration tests - they need more resources (Memory) thus we only run them in

All points addressed and seems we are finally ready to merge that one !

potiuk · 2021-05-26T09:27:07Z

🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉

aneesh-joseph · 2021-05-26T09:28:42Z

awesome, thank you Jarek 👍

kaxil · 2021-05-26T10:27:20Z

Thanks @aneesh-joseph this is great, it took lot of time and pings but thank you for your patience

malthe · 2022-03-01T12:20:19Z

airflow/models/dag.py

@@ -897,7 +898,9 @@ def get_num_active_runs(self, external_trigger=None, session=None):
        )

        if external_trigger is not None:
-            query = query.filter(DagRun.external_trigger == external_trigger)
+            query = query.filter(
+                DagRun.external_trigger == (expression.true() if external_trigger else expression.false())


Why is this necessary? What is the type of external_trigger here to require this workaround?

malthe · 2022-03-01T12:21:02Z

airflow/jobs/scheduler_job.py

+                [(dm.dag_id, dm.next_dagrun) for dm in dag_models]
+            )
+
+        active_dagruns = (


Why not use a join here or CTE rather than making multiple queries?

Good idea. Feel free to give it a shot if you like :)

boring-cyborg bot added the area:dev-tools label Jul 24, 2020

kaxil reviewed Jul 24, 2020

View reviewed changes

Dockerfile.ci Outdated Show resolved Hide resolved

This was referenced Jul 24, 2020

Scheduler errors when using MSSQL as the backend database #9926

Closed

Fix mssql errors when fetching paused dag ids #9948

Closed

potiuk reviewed Jul 24, 2020

View reviewed changes

Dockerfile.ci Outdated Show resolved Hide resolved

mik-laj changed the title ~~improve compatibility with mssql~~ Improve compatibility with mssql Jul 25, 2020

aneesh-joseph force-pushed the improve-mssql-compat branch from 09dff0c to 4ddaadb Compare July 27, 2020 13:57

feluelle reviewed Jul 27, 2020

View reviewed changes

aneesh-joseph force-pushed the improve-mssql-compat branch 3 times, most recently from b4bfc73 to 23b6831 Compare July 29, 2020 06:21

aneesh-joseph force-pushed the improve-mssql-compat branch 4 times, most recently from 25b5ace to 4eb8032 Compare July 30, 2020 13:33

aneesh-joseph and others added 11 commits May 26, 2021 07:14

combine mssql specific migrations

a9e1a04

revert offset changes

823cd1b

fix smart sensor for mssql

4af7bbe

fix mssql CI trigger

39223e4

fixup! fix smart sensor for mssql

23c3566

add experimental MSSQL support details into docs

f00e4d6

incorporate review comments

6b90ccb

incorporate review comments

ff76c41

Apply suggestions from code review

cfb2194

Apply suggestions from code review

51b33de

fixup method names

cfb162e

aneesh-joseph force-pushed the improve-mssql-compat branch from 3ba9344 to cfb162e Compare May 26, 2021 01:45

add back poituk's fix which I removed in error with a force push

f12e007

potiuk approved these changes May 26, 2021

View reviewed changes

potiuk merged commit f2b7d05 into apache:master May 26, 2021

aneesh-joseph deleted the improve-mssql-compat branch May 26, 2021 09:28

ashb modified the milestones: Airflow 2.1.1, Airflow 2.2 Jun 2, 2021

potiuk mentioned this pull request Jul 9, 2021

Add tests suite for MariaDB 10.6+ and fix incompatibilities #16907

Closed

mik-laj mentioned this pull request Jul 15, 2021

Missing documentation about MSSQL setting up #17018

Closed

potiuk mentioned this pull request Sep 24, 2021

Support for MariaDB >= 10.6 #18506

Closed

malthe reviewed Mar 1, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve compatibility with mssql #9973

Improve compatibility with mssql #9973

aneesh-joseph commented Jul 24, 2020 •

edited

Loading

kaxil left a comment

kaxil left a comment

potiuk left a comment

aneesh-joseph commented Jul 24, 2020

potiuk commented Jul 24, 2020

potiuk commented Jul 24, 2020 •

edited

Loading

aneesh-joseph commented Jul 24, 2020

potiuk commented Jul 24, 2020

kaxil commented Jul 24, 2020 •

edited

Loading

potiuk commented Jul 24, 2020

kaxil commented Jul 24, 2020

feluelle left a comment

jarkkorantala commented Jul 28, 2020

aneesh-joseph commented Jul 28, 2020 •

edited

Loading

jarkkorantala commented Jul 28, 2020 •

edited

Loading

aneesh-joseph commented Jul 29, 2020 •

edited

Loading

aneesh-joseph commented May 26, 2021

potiuk commented May 26, 2021

potiuk commented May 26, 2021

aneesh-joseph commented May 26, 2021

kaxil commented May 26, 2021

malthe Mar 1, 2022

malthe Mar 1, 2022

ashb Mar 2, 2022

Improve compatibility with mssql #9973

Improve compatibility with mssql #9973

Conversation

aneesh-joseph commented Jul 24, 2020 • edited Loading

kaxil left a comment

Choose a reason for hiding this comment

kaxil left a comment

Choose a reason for hiding this comment

potiuk left a comment

Choose a reason for hiding this comment

aneesh-joseph commented Jul 24, 2020

potiuk commented Jul 24, 2020

potiuk commented Jul 24, 2020 • edited Loading

aneesh-joseph commented Jul 24, 2020

potiuk commented Jul 24, 2020

kaxil commented Jul 24, 2020 • edited Loading

potiuk commented Jul 24, 2020

kaxil commented Jul 24, 2020

feluelle left a comment

Choose a reason for hiding this comment

jarkkorantala commented Jul 28, 2020

aneesh-joseph commented Jul 28, 2020 • edited Loading

jarkkorantala commented Jul 28, 2020 • edited Loading

aneesh-joseph commented Jul 29, 2020 • edited Loading

aneesh-joseph commented May 26, 2021

potiuk commented May 26, 2021

potiuk commented May 26, 2021

aneesh-joseph commented May 26, 2021

kaxil commented May 26, 2021

malthe Mar 1, 2022

Choose a reason for hiding this comment

malthe Mar 1, 2022

Choose a reason for hiding this comment

ashb Mar 2, 2022

Choose a reason for hiding this comment

aneesh-joseph commented Jul 24, 2020 •

edited

Loading

potiuk commented Jul 24, 2020 •

edited

Loading

kaxil commented Jul 24, 2020 •

edited

Loading

aneesh-joseph commented Jul 28, 2020 •

edited

Loading

jarkkorantala commented Jul 28, 2020 •

edited

Loading

aneesh-joseph commented Jul 29, 2020 •

edited

Loading