Skip to content
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

Fix MySQL connection issue when server misconfigured #15670

Conversation

findepi
Copy link
Member

@findepi findepi commented Jan 11, 2023

When MySQL server has server_time_zone set to e.g. Coordinated Universal Time, the connection couldn't be made. Use UTC instead of
relying on the server system time zone.

The unsupported type case is handled at the end of the method, since
2471b68 commit.
@cla-bot cla-bot bot added the cla-signed label Jan 11, 2023
connectionProperties.setProperty("connectionTimeZone", "SERVER");
// Try to make MySQL timestamps work (See "Solution 2a" at https://dev.mysql.com/doc/connector-j/8.0/en/connector-j-time-instants.html and around)
// without relying on server time zone (which may be configured to be totally unusable).
// See also TODO (https://github.com/trinodb/trino/issues/15668) rethink how timestamps are mapped
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@findepi findepi added the no-release-notes This pull request does not require release notes entry label Jan 11, 2023
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % comment update (since we no longer use "Solution 2a")

// See "Solution 2a" at https://dev.mysql.com/doc/connector-j/8.0/en/connector-j-time-instants.html - these properties are required to preserve point-in-time when operating with MySQL TIMESTAMP types
connectionProperties.setProperty("preserveInstants", "true");
connectionProperties.setProperty("connectionTimeZone", "SERVER");
// Try to make MySQL timestamps work (See "Solution 2a" at https://dev.mysql.com/doc/connector-j/8.0/en/connector-j-time-instants.html and around)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Try to make MySQL timestamps work (See "Solution 2a" at https://dev.mysql.com/doc/connector-j/8.0/en/connector-j-time-instants.html and around)
// Try to make MySQL timestamps work (See https://dev.mysql.com/doc/connector-j/8.0/en/connector-j-time-instants.html)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@hashhar
Copy link
Member

hashhar commented Jan 11, 2023

A TODO to test with server in non-UTC zone should also be added to the same place probably.

@findepi
Copy link
Member Author

findepi commented Jan 12, 2023

since we no longer use "Solution 2a"

i am aware, just wanted to make it easier to locate piece of text on that page

When MySQL server has `server_time_zone` set to e.g. `Coordinated
Universal Time`, the connection couldn't be made. Use UTC instead of
relying on the server system time zone.
@findepi findepi force-pushed the findepi/fix-mysql-connection-issue-when-server-misconfigured-8196a5 branch from 728e741 to 70009be Compare January 12, 2023 17:40
@findepi
Copy link
Member Author

findepi commented Jan 12, 2023

A TODO to test with server in non-UTC zone should also be added to the same place probably.

added

@findepi findepi merged commit 7d32ed3 into trinodb:master Jan 12, 2023
@findepi findepi deleted the findepi/fix-mysql-connection-issue-when-server-misconfigured-8196a5 branch January 12, 2023 17:40
@github-actions github-actions bot added this to the 406 milestone Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed no-release-notes This pull request does not require release notes entry
Development

Successfully merging this pull request may close these issues.

3 participants