Skip to content

Increase Health Check parameters for mssql integration#39877

Merged
potiuk merged 1 commit intoapache:mainfrom
potiuk:increase-health-check-values-for-mssql
May 27, 2024
Merged

Increase Health Check parameters for mssql integration#39877
potiuk merged 1 commit intoapache:mainfrom
potiuk:increase-health-check-values-for-mssql

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented May 27, 2024

The values for mssql integration were too low, particularly the timeout of 3s was far too low. Per documentation:
https://github.com/docker/cli/blob/5bf86c198488c7f78f2ffd05a8a2441f719d3b98/docs/reference/builder.md#healthcheck

Increasing the values shoudl bring stability back.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

The values for mssql integration were too low, particularly the
timeout of 3s was far too low. Per documentation:
https://github.com/docker/cli/blob/5bf86c198488c7f78f2ffd05a8a2441f719d3b98/docs/reference/builder.md#healthcheck

Increasing the values shoudl bring stability back.
@potiuk potiuk requested a review from ashb as a code owner May 27, 2024 15:01
@potiuk potiuk requested a review from Taragolis May 27, 2024 15:01
@potiuk
Copy link
Member Author

potiuk commented May 27, 2024

CC: @shahar1

@potiuk potiuk requested a review from vincbeck May 27, 2024 15:03
@potiuk
Copy link
Member Author

potiuk commented May 27, 2024

Currently it randomly fails - for example https://github.com/apache/airflow/actions/runs/9256599429/job/25463195510?pr=39851

@potiuk potiuk merged commit 8b3ec45 into apache:main May 27, 2024
@potiuk potiuk deleted the increase-health-check-values-for-mssql branch May 27, 2024 15:06
@potiuk
Copy link
Member Author

potiuk commented May 27, 2024

BTW. Those values are pure guesses :), so it might not help either (but they are high enough it should if this is really a problem with slow startup time for mssql

@shahar1
Copy link
Contributor

shahar1 commented May 27, 2024

Thanks!
Now that I think of it, when I implemented the test - I noticed that there were times that the health check failed randomly on start when loading it with breeze. I assume that adding time would stable it.

Some related reading materials that I found:
https://stackoverflow.com/questions/60539114/how-to-wait-for-mssql-in-docker-compose

@shahar1
Copy link
Contributor

shahar1 commented May 27, 2024

https://github.com/apache/airflow/actions/runs/9256982108/job/25464448138

It's still not happy. Maybe we could copy from this script (taken from Stack Overflow's link above) as the health check entry point?

@potiuk
Copy link
Member Author

potiuk commented May 27, 2024

Next attempt - let's see. In the logs you can enable showing timestamp and I saw a few successful tests where it took ~18 s for the mssql container to start - dangerously close to 20s we had.

fdemiane pushed a commit to fdemiane/airflow that referenced this pull request Jun 6, 2024
The values for mssql integration were too low, particularly the
timeout of 3s was far too low. Per documentation:
https://github.com/docker/cli/blob/5bf86c198488c7f78f2ffd05a8a2441f719d3b98/docs/reference/builder.md#healthcheck

Increasing the values shoudl bring stability back.
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
The values for mssql integration were too low, particularly the
timeout of 3s was far too low. Per documentation:
https://github.com/docker/cli/blob/5bf86c198488c7f78f2ffd05a8a2441f719d3b98/docs/reference/builder.md#healthcheck

Increasing the values shoudl bring stability back.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants