-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-29567][TESTS] Update JDBC Integration Test Docker Images #26224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
There is some suspicious part. I'll reopen this later. |
|
Test build #112527 has finished for PR 26224 at commit
|
|
Test build #112528 has finished for PR 26224 at commit
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems fine.
|
Thank you for review, @viirya ! |
|
Test build #112562 has finished for PR 26224 at commit
|
|
retest this please |
| } | ||
|
|
||
| override def dataPreparation(conn: Connection): Unit = { | ||
| // Since MySQL 5.7.14+, we need to disable strict mode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just out of curiosity; why we need to disable the mode in this mysql?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in the PR description, MySQL baned 0000-00-00 00:00:00. There is a SQL mode for that. I enumerated and tested all versions. It starts to ban on 5.7.14.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I missed that. thanks.
| class PostgresIntegrationSuite extends DockerJDBCIntegrationSuite { | ||
| override val db = new DatabaseOnDocker { | ||
| override val imageName = "postgres:11.4" | ||
| override val imageName = "postgres:12.0-alpine" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
12.0-alpine instead of 12.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This is another improvement. It reduces the image size a lot, 312MB -> 72.8MB. This is good for CI/CI testing environment.
$ docker images | grep postgres
postgres 12.0-alpine 5b681acb1cfc 2 days ago 72.8MB
postgres 11.4 53912975086f 3 months ago 312MB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add this to the PR description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Yea, it looks nice.
|
Thank you for review, @maropu ! |
|
Anyway, I checked this pr worked correctly on my local env. |
|
Thank you again, @maropu ! |
|
Test build #112567 has finished for PR 26224 at commit
|
What changes were proposed in this pull request?
This PR updates JDBC Integration Test DBMS Docker Images.
MySQL,SET GLOBAL sql_mode = ''is added to disable all strict modes becausetest("Basic write test")creates a table like the following. The latest MySQL rejects0000-00-00 00:00:00as TIMESTAMP and causes the test case failure.PostgreSQL, I chose the smallest image in12releases. It reduces the image size a lot,312MB->72.8MB. This is good for CI/CI testing environment.Note that
MsSqlServer, we are using2017-GA-ubuntuand the next version2019-CTP3.2-ubuntuis stillCommunity Technology Previewstatus.DB2andOracle, the official images are not available.Why are the changes needed?
This is to make it sure we are testing with the latest DBMS images during preparing
3.0.0.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Since this is the integration test, we need to run this manually.