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/02 23:54:51 UTC
[GitHub] [airflow] kaxil opened a new pull request #11247: Enable MySQL 8 CI jobs
kaxil opened a new pull request #11247:
URL: https://github.com/apache/airflow/pull/11247
closes https://github.com/apache/airflow/issues/11164
<!--
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 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/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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 a change in pull request #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11247:
URL: https://github.com/apache/airflow/pull/11247#discussion_r499183943
##########
File path: scripts/docker/install_mysql.sh
##########
@@ -50,6 +50,7 @@ if [[ ${INSTALL_MYSQL_CLIENT:="true"} == "true" ]]; then
gpgconf --kill all
rm -rf "${GNUPGHOME}"
apt-key list > /dev/null
+ echo "deb http://repo.mysql.com/apt/debian/ buster mysql-5.7" | tee -a /etc/apt/sources.list.d/mysql.list
Review comment:
But yeah.. Maybe that will help :)
----------------------------------------------------------------
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] kaxil commented on a change in pull request #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11247:
URL: https://github.com/apache/airflow/pull/11247#discussion_r499184141
##########
File path: scripts/docker/install_mysql.sh
##########
@@ -50,6 +50,7 @@ if [[ ${INSTALL_MYSQL_CLIENT:="true"} == "true" ]]; then
gpgconf --kill all
rm -rf "${GNUPGHOME}"
apt-key list > /dev/null
+ echo "deb http://repo.mysql.com/apt/debian/ buster mysql-5.7" | tee -a /etc/apt/sources.list.d/mysql.list
Review comment:
Yeah I tried locally, basically, the `libmysqlclient20` was MySQL 5.7 only so the "Build image" step was failing initially. But should work now 🤞 .
I am just trying to keep both but yeah, might remove 5.7 if it is totally compatible
----------------------------------------------------------------
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] kaxil commented on a change in pull request #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #11247:
URL: https://github.com/apache/airflow/pull/11247#discussion_r499130543
##########
File path: scripts/docker/install_mysql.sh
##########
@@ -51,6 +51,7 @@ if [[ ${INSTALL_MYSQL_CLIENT:="true"} == "true" ]]; then
rm -rf "${GNUPGHOME}"
apt-key list > /dev/null
echo "deb http://repo.mysql.com/apt/debian/ stretch mysql-5.7" | tee -a /etc/apt/sources.list.d/mysql.list
+ echo "deb http://repo.mysql.com/apt/debian/ stretch mysql-8.0" | tee -a /etc/apt/sources.list.d/mysql.list
Review comment:
oh yes
----------------------------------------------------------------
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 #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11247:
URL: https://github.com/apache/airflow/pull/11247#issuecomment-703165777
FYI. Hey @kaxil - I just updated to buster 5.7 in the v1-10-test branch and it seems to work fine there (for example here: https://github.com/apache/airflow/actions/runs/286961639) so I do not think it is the buster version. I needed to change it there to solve a missing mysqlclient20 library in 1.10 3.5 prod image version in upgrade-checl PRs, Seems that this is fixed now (but I want to confirm that). I also cherry-picked the latest changes for ci/breeze/scripts to v1-10-test.
If MySQL8 still does not work on Monday - seems that I am close to finishing some of my obligations and might return to pure 2.0 Airflow sooner than I thought, but I would like to focus on the separation of the providers, but maybe we could brainstorm on Monday or earlier what could be the reason/fix if you still have problem with 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] kaxil commented on pull request #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #11247:
URL: https://github.com/apache/airflow/pull/11247#issuecomment-703089031
> Maybe changing back to stretch will help? At least to find out the cause? It could also be some mysql8 client incompatibilities though.
Yeah I am going to try 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11247:
URL: https://github.com/apache/airflow/pull/11247#discussion_r499128948
##########
File path: scripts/docker/install_mysql.sh
##########
@@ -51,6 +51,7 @@ if [[ ${INSTALL_MYSQL_CLIENT:="true"} == "true" ]]; then
rm -rf "${GNUPGHOME}"
apt-key list > /dev/null
echo "deb http://repo.mysql.com/apt/debian/ stretch mysql-5.7" | tee -a /etc/apt/sources.list.d/mysql.list
+ echo "deb http://repo.mysql.com/apt/debian/ stretch mysql-8.0" | tee -a /etc/apt/sources.list.d/mysql.list
Review comment:
Wy PR baby want to switch to buster from stretch since we are using buster as base.
----------------------------------------------------------------
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] kaxil commented on pull request #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #11247:
URL: https://github.com/apache/airflow/pull/11247#issuecomment-703227988
@potiuk CI is green
----------------------------------------------------------------
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 #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11247:
URL: https://github.com/apache/airflow/pull/11247#issuecomment-703087283
I believe those tests were cancelled on timeout - I think we have 60 minutes of timeout for the test jobs and some tests could have been hanging there
----------------------------------------------------------------
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 edited a comment on pull request #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #11247:
URL: https://github.com/apache/airflow/pull/11247#issuecomment-703087897
Maybe changing back to stretch will help? At least to find out the cause? It could also be some mysql8 client incompatibilities though.
----------------------------------------------------------------
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 #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11247:
URL: https://github.com/apache/airflow/pull/11247#issuecomment-703087463
(commenting from my phone so not sure if that was 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 #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11247:
URL: https://github.com/apache/airflow/pull/11247#issuecomment-703243456
Cool !
----------------------------------------------------------------
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 a change in pull request #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11247:
URL: https://github.com/apache/airflow/pull/11247#discussion_r499184435
##########
File path: scripts/docker/install_mysql.sh
##########
@@ -50,6 +50,7 @@ if [[ ${INSTALL_MYSQL_CLIENT:="true"} == "true" ]]; then
gpgconf --kill all
rm -rf "${GNUPGHOME}"
apt-key list > /dev/null
+ echo "deb http://repo.mysql.com/apt/debian/ buster mysql-5.7" | tee -a /etc/apt/sources.list.d/mysql.list
Review comment:
I think only one will be used anyway if both are installed with both DBs (not sure which one though)
----------------------------------------------------------------
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 a change in pull request #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11247:
URL: https://github.com/apache/airflow/pull/11247#discussion_r499128948
##########
File path: scripts/docker/install_mysql.sh
##########
@@ -51,6 +51,7 @@ if [[ ${INSTALL_MYSQL_CLIENT:="true"} == "true" ]]; then
rm -rf "${GNUPGHOME}"
apt-key list > /dev/null
echo "deb http://repo.mysql.com/apt/debian/ stretch mysql-5.7" | tee -a /etc/apt/sources.list.d/mysql.list
+ echo "deb http://repo.mysql.com/apt/debian/ stretch mysql-8.0" | tee -a /etc/apt/sources.list.d/mysql.list
Review comment:
Wy probably want to switch to buster from stretch since we are using buster as base.
----------------------------------------------------------------
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] kaxil edited a comment on pull request #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
kaxil edited a comment on pull request #11247:
URL: https://github.com/apache/airflow/pull/11247#issuecomment-703176748
The following test hangs for MySQL 8
```
tests/providers/mysql/hooks/test_mysql.py::TestMySql::test_mysql_hook_test_bulk_load_0_mysqlclient
tests/providers/mysql/hooks/test_mysql.py::TestMySql::test_mysql_hook_test_bulk_load_1_mysql_connector_python
```
Need to debug further
----------------------------------------------------------------
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] kaxil commented on pull request #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #11247:
URL: https://github.com/apache/airflow/pull/11247#issuecomment-703176748
The following test hangs for MySQL 8
```
tests/providers/mysql/hooks/test_mysql.py::TestMySql::test_mysql_hook_test_bulk_load
```
Need to debug further
----------------------------------------------------------------
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 #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11247:
URL: https://github.com/apache/airflow/pull/11247#issuecomment-703076211
There was a transient server error in the build job. I restarted 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 a change in pull request #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #11247:
URL: https://github.com/apache/airflow/pull/11247#discussion_r499183893
##########
File path: scripts/docker/install_mysql.sh
##########
@@ -50,6 +50,7 @@ if [[ ${INSTALL_MYSQL_CLIENT:="true"} == "true" ]]; then
gpgconf --kill all
rm -rf "${GNUPGHOME}"
apt-key list > /dev/null
+ echo "deb http://repo.mysql.com/apt/debian/ buster mysql-5.7" | tee -a /etc/apt/sources.list.d/mysql.list
Review comment:
I think MYSQL8 client libs should work with both 5.7 and 8 - MySQL client libraries are meant to be fully backwards compatible - in v1-10-test we have now 5.7 client libraries only and they work fine with both 5.6 and 5.7 versions.
----------------------------------------------------------------
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 #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #11247:
URL: https://github.com/apache/airflow/pull/11247#issuecomment-703087897
Maybe changing back to stretch will help? At least to find out the cause? It could also be somy mysql8 client incompatibilities though.
----------------------------------------------------------------
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 merged pull request #11247: Enable MySQL 8 CI jobs
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #11247:
URL: https://github.com/apache/airflow/pull/11247
----------------------------------------------------------------
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