You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "potiuk (via GitHub)" <gi...@apache.org> on 2023/09/04 11:29:11 UTC
[GitHub] [airflow] potiuk opened a new pull request, #34075: Attempt to fix the flaky TestTriggererJob test
potiuk opened a new pull request, #34075:
URL: https://github.com/apache/airflow/pull/34075
As documented in #33323, we have frequent failures of the flaky triggerer job tests.
The flaky failures are about some errors when we close all the sessions in teardown of the test. It turns out that the tests had side-effect - they have not waited for the TriggererJob thread to complete, they merely marked them to be stopped, but they have not waited for those to complete - this is quite plausible explanation of the flaky test failures - since those threads have 1 second sleep, it's more than likely that the session has been created and used by the thread while the teardown has been attempting to close all the sessions.
This side effect could also have an effect for other tests that were run after - because in a busy test run machine, the side effects could propagate further than just to the teardown, so it could also explain why sometimes (very rarely) other job tests failed with similar errors.
The fix is to join the runner after marking it to be stopped.
Fixes: #33323 (Hopefully)
<!--
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.
-->
<!--
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 an 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/
-->
<!-- Please keep an empty line above the dashes. -->
---
**^ 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 changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+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 a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
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 #34075: Attempt to fix the flaky TestTriggererJob test
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #34075:
URL: https://github.com/apache/airflow/pull/34075#issuecomment-1705383169
cc: @ephraimbuddy -> should be good to cherry-pick
--
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 #34075: Attempt to fix the flaky TestTriggererJob test
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #34075:
URL: https://github.com/apache/airflow/pull/34075
--
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 #34075: Attempt to fix the flaky TestTriggererJob test
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #34075:
URL: https://github.com/apache/airflow/pull/34075#issuecomment-1705263978
I think it will. The way it was before had almost guarantee that those races will happen because we have not waited for the thread at all.
Here we give 30 seconds (way more than the 1 sec. Idle) for the thread to complete. And this is all about to make it far far less probable and 'fast enough' even if the thread hangs for some reason or the machine is very, very busy.
In most cases even if the thread will hang for 30 s and we won't complete waiting, nothing bad will happen. We would have to be really unlucky to get the thread do something with session while we are closing it. Previously it was just the question of how close to the end of the 1s sleep the thread was so If the 'race window' was say 10ms - we woud have 1% chance to hit it. With 30 seconds time for the thread to complete IMHO the probability is wery low.
If we have infinite join and the thread does not complete, then we will fail at 60s anyway with test timeout, and the test would fail in this case - even if it would have side effects would be harmless, so I prefer to just 'let it go' after half the time and continue hoping that the side effects won't kick in.
--
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 #34075: Attempt to fix the flaky TestTriggererJob test
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #34075:
URL: https://github.com/apache/airflow/pull/34075#issuecomment-1705270592
I think it will. The way it was before had almost guarantee that those races will happen because we have not waited for the thread at all.
Here we give 30 seconds (way more than the 1 sec. Idle) for the thread to complete. And this is all about to make it far far less probable and 'fast enough' even if the thread hangs for some reason or the machine is very, very busy.
In most cases even if the thread will hang for 30 s and we won't complete waiting, nothing bad will happen. We would have to be really unlucky to get the thread do something with session while we are closing it. Previously it was just the question of how close to the end of the 1s sleep the thread was so If the 'race window' was say 10ms - we woud have 1% chance to hit it. With 30 seconds time for the thread to complete IMHO the probability is wery low.
If we have infinite join and the thread does not complete, then we will fail at 60s anyway with test timeout, and the test would fail in this case - even if it would have side effects would be harmless, so I prefer to just 'let it go' after half the time and continue hoping that the side effects won't kick in.
And yeah likely having separate process is 'better' but I wanted to have a quick fix now - to Alo make it easy to cherry-pick to 2.7
--
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] Taragolis commented on pull request #34075: Attempt to fix the flaky TestTriggererJob test
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #34075:
URL: https://github.com/apache/airflow/pull/34075#issuecomment-1705136080
I'm not sure this would help, because timeout it is only for how long block Main Thread, but thread might still exists.
In the other case if parameter `timeout` not set or None then it would block Main Thread until required thread completed.
Some example
```python
from threading import Thread
from time import sleep
def infinity():
while True:
sleep(1)
if __name__ == "__main__":
thr = Thread(target=infinity)
thr.start()
print("Alive? ", thr.is_alive())
thr.join(5)
print("5 second? I'm stile Alive! ", thr.is_alive())
thr.join(30)
print("30 second? Nope! I'm stile Alive! ", thr.is_alive())
print("Let's block main thread, but this would not help either.")
thr.join()
```
I assume that we should do something on Triggerer side, however killing thread it's not something straight forward, last time when I tried to do that stuff, with events. queues and etc I lost my mind, and I'm not sure that I recovered after that.
--
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] Taragolis commented on pull request #34075: Attempt to fix the flaky TestTriggererJob test
Posted by "Taragolis (via GitHub)" <gi...@apache.org>.
Taragolis commented on PR #34075:
URL: https://github.com/apache/airflow/pull/34075#issuecomment-1705220536
Some additional "simulation" within pytest and long living threads
If run entire module in Pytest, all tests will failed:
- `TestMultiThreading::test_thread_is_alive` - Thread still alive
- `TestMultiThreading::test_check_threads` - Thread with name `"foo-bar"` still exists
- `TestOtherTest::test_check_threads` - Thread with name `"foo-bar"` still exists
```python
import threading
from time import sleep
class TestMultiThreading:
def test_thread_is_alive(self):
def infinity():
while True:
sleep(1)
thr = threading.Thread(target=infinity, name="foo-bar")
try:
thr.start()
finally:
thr.join(2)
assert not thr.is_alive()
def test_check_threads(self):
threads_names = [thread.name for thread in threading.enumerate()]
assert "foo-bar" not in threads_names, threads_names
class TestOtherTest:
def test_check_threads(self):
threads_names = [thread.name for thread in threading.enumerate()]
assert "foo-bar" not in threads_names, threads_names
```
That mean if our side effect that we block main thread and as result we ignore all signals, then solution with timeout would work.
But if our problem that thread existed after test completed and this affect other test, then solution with timeout would not work. And better spawn separate process which would run `TriggererJob` and kill that process in the end of the test.
--
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