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/08/23 22:16:49 UTC
[GitHub] [airflow] jedcunningham opened a new pull request #17797: Fix broken MSSQL test
jedcunningham opened a new pull request #17797:
URL: https://github.com/apache/airflow/pull/17797
This broken test was causing the next test to use the db to fail. Also,
by not ignoring exceptions here we let the failure be exposed where
its broken, not in the next test that happens to run.
This was causing `TestBaseSensor.test_fail` to fail with:
```
pyodbc.ProgrammingError: ('42000', '[42000] [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]The server failed to resume the transaction. Desc:3300000006. (3971) (SQLExecDirectW)')
```
--
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 #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17797:
URL: https://github.com/apache/airflow/pull/17797#discussion_r694367348
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -228,6 +225,8 @@ def test_on_kill(self):
for process in processes:
assert not psutil.pid_exists(process.pid), f"{process} is still alive"
+ session.close()
Review comment:
Maybe simply:
```
from contextlib import closing
with closing(new Session) as session:
--
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 #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17797:
URL: https://github.com/apache/airflow/pull/17797#discussion_r694661249
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -228,6 +225,8 @@ def test_on_kill(self):
for process in processes:
assert not psutil.pid_exists(process.pid), f"{process} is still alive"
+ session.close()
Review comment:
I also still remember this: `ADD X TO Y GIVING Z.` (obligatory . at the end). Guess what language it is.
--
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] jedcunningham merged pull request #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
jedcunningham merged pull request #17797:
URL: https://github.com/apache/airflow/pull/17797
--
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] jedcunningham commented on a change in pull request #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #17797:
URL: https://github.com/apache/airflow/pull/17797#discussion_r694931041
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -228,6 +225,8 @@ def test_on_kill(self):
for process in processes:
assert not psutil.pid_exists(process.pid), f"{process} is still alive"
+ session.close()
Review comment:
Fixup commit added that uses `create_session` 👍
--
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] jedcunningham commented on a change in pull request #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #17797:
URL: https://github.com/apache/airflow/pull/17797#discussion_r695890136
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -181,40 +179,40 @@ def test_on_kill(self):
except OSError:
pass
- dagbag = models.DagBag(
+ dagbag = DagBag(
dag_folder=TEST_DAG_FOLDER,
include_examples=False,
)
dag = dagbag.dags.get('test_on_kill')
task = dag.get_task('task1')
- session = settings.Session()
+ with create_session() as session:
+ dag.clear()
+ dag.create_dagrun(
+ run_id="test",
+ state=State.RUNNING,
+ execution_date=DEFAULT_DATE,
+ start_date=DEFAULT_DATE,
+ session=session,
+ )
+ ti = TaskInstance(task=task, execution_date=DEFAULT_DATE)
+ job1 = LocalTaskJob(task_instance=ti, ignore_ti_state=True)
- dag.clear()
- dag.create_dagrun(
- run_id="test",
- state=State.RUNNING,
- execution_date=DEFAULT_DATE,
- start_date=DEFAULT_DATE,
- session=session,
- )
- ti = TI(task=task, execution_date=DEFAULT_DATE)
- job1 = LocalTaskJob(task_instance=ti, ignore_ti_state=True)
- session.commit()
+ runner = StandardTaskRunner(job1)
+ runner.start()
- runner = StandardTaskRunner(job1)
- runner.start()
+ # give the task some time to startup
+ time.sleep(3)
- # give the task some time to startup
- time.sleep(3)
+ pgid = os.getpgid(runner.process.pid)
+ assert pgid > 0
+ assert pgid != os.getpgid(0), "Task should be in a different process group to us"
- pgid = os.getpgid(runner.process.pid)
- assert pgid > 0
- assert pgid != os.getpgid(0), "Task should be in a different process group to us"
+ processes = list(self._procs_in_pgroup(pgid))
- processes = list(self._procs_in_pgroup(pgid))
+ runner.terminate()
- runner.terminate()
+ session.close() # explicitly close as `create_session`s commit will blow up otherwise
Review comment:
`rollback` doesn't work, `terminate` seemingly already kills the transaction.
--
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] jedcunningham commented on a change in pull request #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #17797:
URL: https://github.com/apache/airflow/pull/17797#discussion_r694361191
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -228,6 +225,8 @@ def test_on_kill(self):
for process in processes:
assert not psutil.pid_exists(process.pid), f"{process} is still alive"
+ session.close()
Review comment:
I can convert this to `createSession` which would do that, but calling `close` like this is pretty common in our test suite.
--
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 #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17797:
URL: https://github.com/apache/airflow/pull/17797#discussion_r694367348
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -228,6 +225,8 @@ def test_on_kill(self):
for process in processes:
assert not psutil.pid_exists(process.pid), f"{process} is still alive"
+ session.close()
Review comment:
Maybe simply:
```
from contextlib import closing
with closing(new Session) as session:
....
```
That would avoid creating an explicit context manager.
--
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] uranusjr commented on pull request #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
uranusjr commented on pull request #17797:
URL: https://github.com/apache/airflow/pull/17797#issuecomment-905061157
Hmm one MSSQL test is failing with
```
pyodbc.Error: ('40001', '[40001] [Microsoft][ODBC Driver 17 for SQL Server][SQL Server]Transaction (Process ID 56)
was deadlocked on lock resources with another process and has been chosen as the deadlock victim. Rerun the
transaction. (1205) (SQLExecDirectW)')
```
--
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] jedcunningham commented on pull request #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on pull request #17797:
URL: https://github.com/apache/airflow/pull/17797#issuecomment-905641274
> Hmm one MSSQL test is failing with
I wasn't able to reproduce that locally, and with it being in `test_scheduler_job.py` makes me suspicious it may simply be another flaky test. Let's see what this run does.
--
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] uranusjr commented on a change in pull request #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17797:
URL: https://github.com/apache/airflow/pull/17797#discussion_r694509540
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -62,15 +62,12 @@ def logging_and_db(self):
(as the test environment does not have enough context for the normal
way to run) and ensures they reset back to normal on the way out.
"""
+ clear_db_runs()
dictConfig(LOGGING_CONFIG)
yield
airflow_logger = logging.getLogger('airflow')
airflow_logger.handlers = []
- try:
- clear_db_runs()
- except Exception:
- # It might happen that we lost connection to the server here so we need to ignore any errors here
- pass
+ clear_db_runs()
Review comment:
Is this related? I’m suspecting the try-except is there for a reason… but we can always add it back anyway.
--
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 #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17797:
URL: https://github.com/apache/airflow/pull/17797#discussion_r695028515
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -181,40 +179,40 @@ def test_on_kill(self):
except OSError:
pass
- dagbag = models.DagBag(
+ dagbag = DagBag(
dag_folder=TEST_DAG_FOLDER,
include_examples=False,
)
dag = dagbag.dags.get('test_on_kill')
task = dag.get_task('task1')
- session = settings.Session()
+ with create_session() as session:
+ dag.clear()
+ dag.create_dagrun(
+ run_id="test",
+ state=State.RUNNING,
+ execution_date=DEFAULT_DATE,
+ start_date=DEFAULT_DATE,
+ session=session,
+ )
+ ti = TaskInstance(task=task, execution_date=DEFAULT_DATE)
+ job1 = LocalTaskJob(task_instance=ti, ignore_ti_state=True)
- dag.clear()
- dag.create_dagrun(
- run_id="test",
- state=State.RUNNING,
- execution_date=DEFAULT_DATE,
- start_date=DEFAULT_DATE,
- session=session,
- )
- ti = TI(task=task, execution_date=DEFAULT_DATE)
- job1 = LocalTaskJob(task_instance=ti, ignore_ti_state=True)
- session.commit()
+ runner = StandardTaskRunner(job1)
+ runner.start()
- runner = StandardTaskRunner(job1)
- runner.start()
+ # give the task some time to startup
+ time.sleep(3)
- # give the task some time to startup
- time.sleep(3)
+ pgid = os.getpgid(runner.process.pid)
+ assert pgid > 0
+ assert pgid != os.getpgid(0), "Task should be in a different process group to us"
- pgid = os.getpgid(runner.process.pid)
- assert pgid > 0
- assert pgid != os.getpgid(0), "Task should be in a different process group to us"
+ processes = list(self._procs_in_pgroup(pgid))
- processes = list(self._procs_in_pgroup(pgid))
+ runner.terminate()
- runner.terminate()
+ session.close() # explicitly close as `create_session`s commit will blow up otherwise
Review comment:
should not rollback() be better here?
--
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] jedcunningham commented on a change in pull request #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
jedcunningham commented on a change in pull request #17797:
URL: https://github.com/apache/airflow/pull/17797#discussion_r694905940
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -62,15 +62,12 @@ def logging_and_db(self):
(as the test environment does not have enough context for the normal
way to run) and ensures they reset back to normal on the way out.
"""
+ clear_db_runs()
dictConfig(LOGGING_CONFIG)
yield
airflow_logger = logging.getLogger('airflow')
airflow_logger.handlers = []
- try:
- clear_db_runs()
- except Exception:
- # It might happen that we lost connection to the server here so we need to ignore any errors here
- pass
+ clear_db_runs()
Review comment:
Kinda. This was why we saw a failure in `TestBaseSensor.test_fail`, which just happened to be the next test that used the db. I'd rather see an exception here during cleanup instead of in an unrelated test as it took me a while to track down which test was responsible.
--
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 #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17797:
URL: https://github.com/apache/airflow/pull/17797#discussion_r694367348
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -228,6 +225,8 @@ def test_on_kill(self):
for process in processes:
assert not psutil.pid_exists(process.pid), f"{process} is still alive"
+ session.close()
Review comment:
Maybe simply:
```
from contextlib import closing
with closing(new Session()) as session:
....
```
That would avoid creating an explicit context manager.
--
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 #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17797:
URL: https://github.com/apache/airflow/pull/17797#discussion_r694363964
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -228,6 +225,8 @@ def test_on_kill(self):
for process in processes:
assert not psutil.pid_exists(process.pid), f"{process} is still alive"
+ session.close()
Review comment:
I see a number of similar uses (but a number of finally ones as well). Actually not having finally might be a reason why we are having side effects/flaky tests so we might want to fix them all.
--
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 #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17797:
URL: https://github.com/apache/airflow/pull/17797#discussion_r694662053
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -228,6 +225,8 @@ def test_on_kill(self):
for process in processes:
assert not psutil.pid_exists(process.pid), f"{process} is still alive"
+ session.close()
Review comment:
That was true `literate programming` :D. which is becoming fashionable again.
--
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 #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17797:
URL: https://github.com/apache/airflow/pull/17797#discussion_r694365694
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -228,6 +225,8 @@ def test_on_kill(self):
for process in processes:
assert not psutil.pid_exists(process.pid), f"{process} is still alive"
+ session.close()
Review comment:
Actually it does call for a `with newSession() as session` ... Should we make ContextManager for that? That would avoid try/finally.
--
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] jedcunningham closed pull request #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
jedcunningham closed pull request #17797:
URL: https://github.com/apache/airflow/pull/17797
--
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] uranusjr commented on a change in pull request #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
uranusjr commented on a change in pull request #17797:
URL: https://github.com/apache/airflow/pull/17797#discussion_r694508782
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -228,6 +225,8 @@ def test_on_kill(self):
for process in processes:
assert not psutil.pid_exists(process.pid), f"{process} is still alive"
+ session.close()
Review comment:
I like how your code blocks are all in a Java-ish language 😆
--
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 #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17797:
URL: https://github.com/apache/airflow/pull/17797#discussion_r694358373
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -228,6 +225,8 @@ def test_on_kill(self):
for process in processes:
assert not psutil.pid_exists(process.pid), f"{process} is still alive"
+ session.close()
Review comment:
Should it be in try/finally clause? Or maybe even setup/tearDown?
--
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] jedcunningham closed pull request #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
jedcunningham closed pull request #17797:
URL: https://github.com/apache/airflow/pull/17797
--
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 #17797: Fix broken MSSQL test
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17797:
URL: https://github.com/apache/airflow/pull/17797#discussion_r694660446
##########
File path: tests/task/task_runner/test_standard_task_runner.py
##########
@@ -228,6 +225,8 @@ def test_on_kill(self):
for process in processes:
assert not psutil.pid_exists(process.pid), f"{process} is still alive"
+ session.close()
Review comment:
some things are never forgotten :D.
--
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