[MySQL] set useLegacyDatetimeCode=false by default.

[hiroyuki-sato]

For future discussion about embulk-input-mysql.
It seems better to set useLegacyDatetimeCode=false by default.

For example, If server use UTC timezone, and client use 'Asia/Tokyo',
a user need to set options: { useLegacyDatetimeCode: false } or similar explicitly.

https://dev.mysql.com/doc/connector-j/5.1/en/connector-j-reference-configuration-properties.html

It's not convenient.

The following references are written in Japanese.

http://qiita.com/katsuyan/items/f22dc5a86522ba3c5652
https://gist.github.com/hiroyuki-sato/dc59e8d45778dac0e0ee417bb27d57a1
http://vividcode.hatenablog.com/entry/mysql/connector-j-5.1-use-legacy-datetime-code

[muga]

I think that we need to change the default value with false before (or when) we upgrade connector-j 6.x for embulk-input-mysql. Because it seems that connector-j 6.x doesn't use the option.

[hiroyuki-sato]

Do we need to update mysql-connector-java, if useLegacyDatetimeCode set to false by default?
(Current mysql-connector-java is 5.1.34)

https://dev.mysql.com/doc/relnotes/connector-j/5.1/en/news-5-1-35.html

A java.sql.date value was stored incorrectly on the server and also returned incorrectly if the client and the server were in different time zones when useLegacyDatetimeCode=false or useTimezone=true. This was due to the time-zone conversion performed by Connector/J on the SQL DATE type. To avoid the issue, a new property noTimezoneConversionForDateType has been created for Connector/J, which is set to “true” by default, preventing Connector/J to perform the kind of time-zone conversion that caused this bug.

[hiroyuki-sato]

@muga @hito4t

I create a summary and proposed PR #104.
And I have two questions.

1. Is it better to change useLegacyDatetimeCode set to false by default?
2. If the No 1. answer is Yes, Is it better to update mysql-connector-java to the latest(5.1.41)?

If both answers are YES, we need the following works.

• Change test code.
• Care useSSL option

Change test code

Some tests failed due to timestamp comparison.

• https://travis-ci.org/embulk/embulk-input-jdbc/builds/225554598#L902

• org.embulk.input.mysql.BasicTest > testTimestamp3

  • Expect: 2015/06/05 05:45:06
  • Test Result: 2015/06/05 08:45:06

org.embulk.input.mysql.BasicTest > testTimestamp3 FAILED
    java.lang.AssertionError: 
    Expected: is "10,,,,,,,,,,,,,2015/06/05 05:45:06,,\n11,99,9999,-99999999,-9999999999999999,1.2345000505447388,1.234567890123,-1234.0,1.2345678901234568E17,5678,xy,2015/06/04,2015/06/04 18:34:56,2015/06/05 05:45:06,09-04-02,2015/06/04 07:02:03.123456\n"
         but: was "10,,,,,,,,,,,,,2015/06/05 08:45:06,,\n11,99,9999,-99999999,-9999999999999999,1.2345000505447388,1.234567890123,-1234.0,1.2345678901234568E17,5678,xy,2015/06/04,2015/06/04 21:34:56,2015/06/05 08:45:06,09-04-02,2015/06/04 10:02:03.123456\n"
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
        at org.junit.Assert.assertThat(Assert.java:956)
        at org.junit.Assert.assertThat(Assert.java:923)
        at org.embulk.input.mysql.BasicTest.testTimestamp3(BasicTest.java:116)

Care useSSL option

We also need to add useSSL option.
When I update mysql-connector-java 5.1.41, I got the following WARNING

Tue Apr 25 20:59:02 JST 2017 WARN: Establishing SSL connection without server's identity verification is not recommended. According to MySQL 5.5.45+, 5.6.26+ and 5.7.6+ requirements SSL connection must be established by default if explicit option isn't set. For compliance with existing applications not using SSL the verifyServerCertificate property is set to 'false'. You need either to explicitly disable SSL by setting useSSL=false, or set useSSL=true and provide truststore for server certificate verification.

[muga]

@hiroyuki-sato Thank you for moving forward this issue. I think that we need to discuss about how we should do first. And let's discuss about useSSL option on another ticket, it's also important topic though.

I'm OK to set useLegacyDatetimeCode=false by default and break the backward compatibility. Because we might need to do that if we will update connector/j 6.0.x for this plugin.

If we set useLegacyDatetimeCode=false by default, as first step, the plugin should show warning messages like "will set useLegacyDatetimeCode=false by default" or something to users who are using this plugin with useLegacyDatetimeCode=true. Then, I'm not sure but, it might be better to set useLegacyDatetimeCode=false by default actually or upgrade connector/j 6.0.x.

How about this? @hito4t @frsyuki

[hito4t]

@hiroyuki-sato @muga @frsyuki
I agree with @muga .
I think useLegacyDatetimeCode should be false, but we should deliberate to break the backward compatibility.
The plugin should show the following warning message before changing default value actually.

• If server timezone and client timezone are different, the plugin will fetch wrong datetime values.
• To correct it, set options: { useLegacyDatetimeCode: false } .
• The plugin will set useLegacyDatetimeCode=false by default in future.

[hiroyuki-sato]

@muga @hito4t
Thank you. Let's implement warning message!.
And I think, it's a good place to announce this change plan at Embulk Meetup#3.

[hiroyuki-sato]

Follow-up #106. It merged into master.
So I close this issue.

[hiroyuki-sato]

I reopened this issue.
Because useLegacyDatetimeCode doesn't set false by default yet.