The MySQL plugin compare server and client timezone #106

hiroyuki-sato · 2017-04-29T14:04:18Z

Reference implementation This comment.

The embulk-input-mysql to compare server and client timezone.

I would like to request comments about this changes.

Questions

How to check server timezone parameter before load data.
- I made before_load method for executing timezone check SQL.
How to get server timezone parameter
- I executed the query select @@system_time_zone for get server timezone.
- Older MySQL version doesn't support @@system_time_zone parameter. (I'll comment later)
- I tried to use getServerTimezoneTZ() method. But It always returns null
How to get the client side parameter.
- I used System.getProperty("user.timezone");
How to compare client and server timezone.
- I used sys_tz.hasSameRules(usr_tz).
- This method return false when I compare GMT+9 and JST (See below)

Same timezone

2017-04-29 21:57:47.042 +0900: Embulk v0.8.18
2017-04-29 21:57:48.972 +0900 [INFO] (0001:preview): Loaded plugin embulk/input/mysql from a load path
2017-04-29 21:57:49.031 +0900 [INFO] (0001:preview): Fetch size is 10000. Using server-side prepared statement.
2017-04-29 21:57:49.446 +0900 [INFO] (0001:preview): Fetch size is 10000. Using server-side prepared statement.
2017-04-29 21:57:49.455 +0900 [WARN] (0001:preview): The plugin will set `useLegacyDatetimeCode=false` by default in future.
2017-04-29 21:57:49.456 +0900 [INFO] (0001:preview): SQL: SELECT * FROM `time_test`
2017-04-29 21:57:49.461 +0900 [INFO] (0001:preview): > 0.00 seconds

Difference timezone

2017-04-29 22:00:55.618 +0900 [INFO] (0001:preview): Loaded plugin embulk/input/mysql from a load path
2017-04-29 22:00:55.671 +0900 [INFO] (0001:preview): Fetch size is 10000. Using server-side prepared statement.
2017-04-29 22:00:56.103 +0900 [INFO] (0001:preview): Fetch size is 10000. Using server-side prepared statement.
2017-04-29 22:00:56.119 +0900 [WARN] (0001:preview): The server timezone and client timezone are different, the plugin will fetch wrong datetime values.
2017-04-29 22:00:56.119 +0900 [WARN] (0001:preview): Use `options: { useLegacyDatetimeCode: false }`
2017-04-29 22:00:56.119 +0900 [WARN] (0001:preview): The plugin will set `useLegacyDatetimeCode=false` by default in future.
2017-04-29 22:00:56.120 +0900 [INFO] (0001:preview): SQL: SELECT * FROM `time_test`
2017-04-29 22:00:56.125 +0900 [INFO] (0001:preview): > 0.00 seconds

Compare timezone

import java.util.TimeZone;

class CompareTimeZone {
  public static void main(String args[]){
    TimeZone tz_jst   = TimeZone.getTimeZone("JST");
    TimeZone tz_tokyo = TimeZone.getTimeZone("Asia/Tokyo");
    TimeZone tz_gmt9  = TimeZone.getTimeZone("GMT+9");

    System.out.println(tz_jst);
    System.out.println(tz_tokyo);
    System.out.println(tz_gmt9);

    System.out.println(tz_jst.hasSameRules(tz_jst));
    System.out.println(tz_jst.hasSameRules(tz_gmt9));

  }
}

sun.util.calendar.ZoneInfo[id="JST",offset=32400000,dstSavings=0,useDaylight=false,transitions=10,lastRule=null]
sun.util.calendar.ZoneInfo[id="Asia/Tokyo",offset=32400000,dstSavings=0,useDaylight=false,transitions=10,lastRule=null]
sun.util.calendar.ZoneInfo[id="GMT+09:00",offset=32400000,dstSavings=0,useDaylight=false,transitions=0,lastRule=null]
true
false

hito4t · 2017-05-02T09:26:55Z

Thank you for the PR!
It will be very helpful if embulk-output-mysql can output such warning messages.

Though select @@system_time_zone may return "SYSTEM", we need to write more codes to detect server timezone.
http://stackoverflow.com/questions/2934258/how-do-i-get-the-current-time-zone-of-mysql

hiroyuki-sato · 2017-05-02T10:22:57Z

@hito4t. Thank you for replying.

It seems @@global.time_zone not @@system_time_zone.
(It seems that @@system_time_zone does not return SYSTEM).

Do you know how to set @@system_time_zone to SYSTEM?
Should we care @@system_time_zone != @@global.time_zone case?

I tried MySQL Server version: 5.0.95.
It's the RPM package on CentOS5.

It @@system_time_zone worked.
It may support on the all of the supported MySQL versions.

mysql> SELECT @@global.time_zone, @@session.time_zone,@@system_time_zone;
+--------------------+---------------------+--------------------+
| @@global.time_zone | @@session.time_zone | @@system_time_zone |
+--------------------+---------------------+--------------------+
| SYSTEM             | SYSTEM              | JST                |
+--------------------+---------------------+--------------------+
1 row in set (0.00 sec)

hito4t · 2017-05-02T11:10:45Z

I'm sorry, I've mistaken.

In my environment (Windows 7 and MySQL 5.6), the string value returned by SELECT @@system_time_zone was corrupt.
(regardless of whether MySQL character set is utf8 or sjis)

I could get right value by the following code.

ResultSet rs = statement.executeQuery("SELECT @@system_time_zone");
if (rs.next()) {
    byte[] b = rs.getBytes(1);
    String tz = new String(b, "MS932");
}

The result value was "東京 (標準時)" (Japanese).
I couldn't get right timezone by TimeZone.getTimeZone("東京 (標準時)");
It returned GMT, not JST.

The value of @@system_time_zone may depend on OS.

hiroyuki-sato · 2017-05-02T11:30:40Z

OMG, we can't use @@system_time_zone.

I'm investigating an alternative idea.

How about this? (need implement TODO part).

~~SELECT IF(TIMEDIFF(NOW(), UTC_TIMESTAMP) = TIME('00:00:00'),"UTC","TODO");~~

I want to output "GMT-07:00".

mysql> SELECT IF(TIMEDIFF(NOW(), UTC_TIMESTAMP) = TIME('00:00:00'),"UTC",concat("GMT",EXTRACT(HOUR FROM (TIMEDIFF(NOW(), UTC_TIMESTAMP))))) as timezone;
+----------+
| timezone |
+----------+
| GMT-7    |
+----------+

mysql> select @@system_time_zone;
+--------------------+
| @@system_time_zone |
+--------------------+
| PDT                |
+--------------------+
1 row in set (0.00 sec)
mysql> select timediff( now(), utc_timestamp());
+-----------------------------------+
| timediff( now(), utc_timestamp()) |
+-----------------------------------+
| -07:00:00                         |
+-----------------------------------+
1 row in set (0.00 sec)

hiroyuki-sato · 2017-05-02T13:15:11Z

@hito4t

Can you try the following SQL?

select IF(timediff(now(),utc_timestamp())=0,'UTC',
  IF(timediff(now(),utc_timestamp())>0,
    CONCAT('GMT+',TIME_FORMAT(timediff(now(),utc_timestamp()),'%h:%m')),
    CONCAT('GMT',TIME_FORMAT(timediff( now(), utc_timestamp()),'%h:%m')))) as offset;

OR

set @timediff = timediff(now(),utc_timestamp());
select IF(@timediff=0,'UTC',
  IF(@timediff>0,
    CONCAT('GMT+',TIME_FORMAT(@timediff,'%h:%m')),
    CONCAT('GMT',TIME_FORMAT(@timediff,'%h:%m')))) as offset;

+--------------------+
| @@system_time_zone |
+--------------------+
| JST                |
+--------------------+
1 row in set (0.00 sec)

mysql> select IF(timediff(now(),utc_timestamp())=0,'UTC',
    ->   IF(timediff(now(),utc_timestamp())>0,
    ->     CONCAT('GMT+',TIME_FORMAT(timediff(now(),utc_timestamp()),'%h:%m')),
    ->     CONCAT('GMT',TIME_FORMAT(timediff( now(), utc_timestamp()),'%h:%m')))) as offset;
+-----------+
| offset    |
+-----------+
| GMT+09:00 |
+-----------+
1 row in set (0.00 sec)

mysql> select @@system_time_zone;
+--------------------+
| @@system_time_zone |
+--------------------+
| PDT                |
+--------------------+
1 row in set (0.00 sec)

mysql> select IF(timediff(now(),utc_timestamp())=0,'UTC',
    ->   IF(timediff(now(),utc_timestamp())>0,
    ->     CONCAT('GMT+',TIME_FORMAT(timediff(now(),utc_timestamp()),'%h:%m')),
    ->     CONCAT('GMT',TIME_FORMAT(timediff( now(), utc_timestamp()),'%h:%m')))) as offset;
+-----------+
| offset    |
+-----------+
| GMT-07:00 |
+-----------+
1 row in set (0.00 sec)

mysql> select @@system_time_zone;
+--------------------+
| @@system_time_zone |
+--------------------+
| UTC                |
+--------------------+
1 row in set (0.00 sec)

mysql> select IF(timediff(now(),utc_timestamp())=0,'UTC',                                                                      ->   IF(timediff(now(),utc_timestamp())>0,
    ->     CONCAT('GMT+',TIME_FORMAT(timediff(now(),utc_timestamp()),'%h:%m')),
    ->     CONCAT('GMT',TIME_FORMAT(timediff( now(), utc_timestamp()),'%h:%m')))) as offset;
+--------+
| offset |
+--------+
| UTC    |
+--------+
1 row in set (0.00 sec)

hiroyuki-sato · 2017-05-03T14:19:22Z

@hito4t

Please take a look again. I calculate GMT offset.

TZ=PST8PDT mysql.server start

mysql> select @@system_time_zone;
+--------------------+
| @@system_time_zone |
+--------------------+
| PDT                |
+--------------------+
1 row in set (0.00 sec)

2017-05-03 23:16:14.928 +0900 [WARN] (0001:preview): The server timezone offset(GMT-07:00) and client timezone(Asia/Tokyo) has different timezone offset. The plugin will fetch wrong datetime values.
2017-05-03 23:16:14.928 +0900 [WARN] (0001:preview): Use `options: { useLegacyDatetimeCode: false }`
2017-05-03 23:16:14.928 +0900 [WARN] (0001:preview): The plugin will set `useLegacyDatetimeCode=false` by default in future.

hito4t · 2017-05-10T01:22:19Z

@hiroyuki-sato

Can you try the following SQL?

select IF(timediff(now(),utc_timestamp())=0,'UTC',
IF(timediff(now(),utc_timestamp())>0,
CONCAT('GMT+',TIME_FORMAT(timediff(now(),utc_timestamp()),'%h:%m')),
CONCAT('GMT',TIME_FORMAT(timediff( now(), utc_timestamp()),'%h:%m')))) as offset;

OR

set @timediff = timediff(now(),utc_timestamp());
select IF(@timediff=0,'UTC',
IF(@timediff>0,
CONCAT('GMT+',TIME_FORMAT(@timediff,'%h:%m')),
CONCAT('GMT',TIME_FORMAT(@timediff,'%h:%m')))) as offset;

Both worked fine!

I think SQL should return timezone offset as number and Java should convert it to timezone string because the timezone strings is interpreted in Java.

hiroyuki-sato · 2017-05-10T02:31:44Z

@hito4t Thank you for your comment.

How do you think this query?

select TIME_TO_SEC(timediff(now(),utc_timestamp()))

Offset	Timezone	GMT
-25200	PST8PDT(PDT)	GMT-07:00
0	UTC	UTC
16200	Asia/Kabul	GMT+04:50
32400	Asia/Tokyo	GMT+09:00

Implementation Idea

SQL part
- Query UTC offset using select TIME_TO_SEC(timediff(now(),utc_timestamp())).
- We need query in seconds not in hour. because some area using half in hour (ex. Asia/Kabul: GMT+04:50 )
- The query result will be 0, -25200, 16200... (called offset sec)
Java Part
- If offset sec is zero, It's UTC.
- Check offset sec is (+/-)
- Calculate hour from offset sec 60sec * 60min
- Calculate min from offset sec
- make a zoneinfo like 'GMT+09:00'

Memo

List of tz database time zones
sudo systemsetup -listtimezones generate timezone list on macOS.

hiroyuki-sato · 2017-05-10T04:25:07Z

@hito4t Please take a look again.

I think I should seperate TimeZone getter (builder?) class like the following.
How do you think?

public TimeZone MySQLTimeZoneBuilder.fromSystemConfig(Connection connection) throws SQLException;
public TimeZone MySQLTimeZoneBuilder.fromUTCOffsetSeconds(int offset);

This is the sample output.

2017-05-10 13:26:00.501 +0900 [WARN] (0001:preview): The server timezone offset(GMT-07:00) and client timezone(Asia/Tokyo) has different timezone offset. The plugin will fetch wrong datetime values.
2017-05-10 13:26:00.501 +0900 [WARN] (0001:preview): Use `options: { useLegacyDatetimeCode: false }`
2017-05-10 13:26:00.501 +0900 [WARN] (0001:preview): The plugin will set `useLegacyDatetimeCode=false` by default in future.

hiroyuki-sato · 2017-05-10T08:42:30Z

@hito4t, Please take a look again.
I made the class MySQLTimeZoneBuilder.

You can change timezone like the following quickly.

set global time_zone = "-07:30";

hito4t

@hiroyuki-sato
Thank you!
Please see review comments.

hito4t · 2017-05-12T02:33:51Z

embulk-input-mysql/src/main/java/org/embulk/input/MySQLTimeZoneBuilder.java

+        //
+        String query = "select TIME_TO_SEC(timediff(now(),utc_timestamp()));";
+        Statement stmt = connection.createStatement();
+        ResultSet rs = stmt.executeQuery(query);


Statement and ResultSet are not closed.
They should be closed in finally clause.

Statement stmt = ... try { ... } finally { stmt.close(); }

Add stmt.close exception handling is still TODO.

hito4t · 2017-05-12T02:37:23Z

embulk-input-mysql/src/main/java/org/embulk/input/MySQLTimeZoneBuilder.java

+        ResultSet rs = stmt.executeQuery(query);
+
+        if (rs.next()) {
+            int offset_seconds = rs.getInt(1);


Class names, method names and variable names (except for constant names) should be CamelCased in Java.
For example, offset_seconds should be offsetSeconds.
http://www.oracle.com/technetwork/java/codeconventions-135099.html

Variable names use camel case like offsetSeconds.

hito4t · 2017-05-12T02:39:05Z

embulk-input-mysql/src/main/java/org/embulk/input/MySQLTimeZoneBuilder.java

+        }
+    }
+
+    public static TimeZone fromGMTOffsetSeconds(int offset_seconds)


Is it necessary to be public?

Change to private.

hito4t · 2017-05-12T02:40:07Z

embulk-input-mysql/src/main/java/org/embulk/input/MySQLTimeZoneBuilder.java

+        int tz_min = abs_offset_sec % ONE_HOUR_SEC / ONE_MIN_SEC;
+        String tz_name = String.format(Locale.ENGLISH, "GMT%s%02d:%02d", sign, tz_hour, tz_min);
+        return TimeZone.getTimeZone(tz_name);
+    }


Does it work for summer time?

As discussed on the Twitter, I used getDSTSavings method like the below.
If this code is better,
I'll change this.

TimeZone clientTimeZone = TimeZone.getDefault(); Date today = new Date(); int clientOffset = clientTimeZone.getRawOffset(); if( clientTimeZone.inDaylightTime(today) ){ clientOffset += clientTimeZone.getDSTSavings(); }

hito4t · 2017-05-12T02:42:15Z

embulk-input-mysql/src/main/java/org/embulk/input/mysql/MySQLInputConnection.java

+        TimeZone svr_tz = MySQLTimeZoneBuilder.fromSystemTimeZone(connection);
+
+        String usr_tz_name = System.getProperty("user.timezone");
+        TimeZone usr_tz = TimeZone.getTimeZone(usr_tz_name);


We can get same value by TimeZone.getDefaultValue().

Changed to TimeZone.getDefaultValue.

hito4t · 2017-05-12T02:45:56Z

embulk-input-mysql/src/main/java/org/embulk/input/mysql/MySQLInputConnection.java

+        //
+        if( svr_tz.getRawOffset() != usr_tz.getRawOffset() ) {
+            logger.warn(String.format(Locale.ENGLISH,
+                    "The server timezone offset(%s) and client timezone(%s) has different timezone offset. The plugin will fetch wrong datetime values.",svr_tz.getID(),usr_tz_name));


How about following message?
The client timezone(%s) is different from the server timezone(%s). ...

Changed to The client timezone(%s) is different from the server timezone(%s).

hito4t · 2017-05-12T02:55:33Z

embulk-input-jdbc/src/main/java/org/embulk/input/jdbc/AbstractJdbcInputPlugin.java

@@ -426,6 +426,8 @@ public TaskReport run(TaskSource taskSource,
        LastRecordStore lastRecordStore = null;

        try (JdbcInputConnection con = newConnection(task)) {
+            con.before_load();


newConnection is called by AbstractJdbcInputPlugin#transaction first, then called by AbstractJdbcInputPlugin#run.
So we should check timezone in the transaction method.

@Override public ConfigDiff transaction(ConfigSource config, InputPlugin.Control control) { ... Schema schema; try (JdbcInputConnection con = newConnection(task)) { // TODO incremental_columns is not set => get primary key schema = setupTask(con, task); } catch (SQLException ex) { throw Throwables.propagate(ex); } return buildNextConfigDiff(task, control.run(task.dump(), schema, 1)); }

Now setupTask method is private, but I think the method is appropriate to check timezone if it is changed to protected.

I changed setupTask to protect and Override setupTask like the below.

@Override protected Schema setupTask(JdbcInputConnection con, PluginTask task) throws SQLException { MySQLInputConnection mySQLCon = (MySQLInputConnection)con; mySQLCon.compareTimeZone(); return super.setupTask(con,task); }

hiroyuki-sato · 2017-05-12T17:03:08Z

@hito4t Thank you for your comment.

I wrote two implementations.
Which is better this PR and TimeZoneComparison version?

hito4t · 2017-05-16T04:56:31Z

@hiroyuki-sato
Thank you!
I think the TimeZoneComparison version is better because codes about TimeZone are put into one class.
Can you implement "// TODO error check"?

hiroyuki-sato · 2017-05-17T06:20:17Z

@hito4t

Please take a look again. I implemented TODO part.
Some users recommend me to set serverTimeZone parameter too.
So I changed the error message like the following.

2017-05-17 15:16:13.994 +0900 [WARN] (0001:preview): The client timezone(Asia/Tokyo) is different from the server timezone(GMT-07:00). The plugin will fetch wrong datetime values.
2017-05-17 15:16:13.994 +0900 [WARN] (0001:preview): Use You may need to set options `useLegacyDatetimeCode` and `serverTimeZone`
2017-05-17 15:16:13.994 +0900 [WARN] (0001:preview): Ex. `options: { useLegacyDatetimeCode: false, serverTimeZone: UTC }`
2017-05-17 15:16:13.994 +0900 [WARN] (0001:preview): The plugin will set `useLegacyDatetimeCode=false` by default in future.

hito4t

@hiroyuki-sato
Thank you.
I have a few comments.
Please check them.

hito4t · 2017-05-19T04:32:37Z

embulk-input-mysql/src/main/java/org/embulk/input/MySQLTimeZoneComparison.java

+            serverTimeZone = getServerTimeZone();
+        }
+        catch (SQLException ex) {
+            logger.error(String.format(Locale.ENGLISH, "SQLException raised %s", ex.toString()));


It should be logger.warn because it is not fatal even if timezone checking fails.

change logger.warn.

hito4t · 2017-05-19T04:35:29Z

embulk-input-mysql/src/main/java/org/embulk/input/MySQLTimeZoneComparison.java

+                return fromGMTOffsetSeconds(offsetSeconds);
+            }
+            else {
+                return null;


If throwing SQLException instead of returning null, the caller doesn't have to check null.

throw SQLException if timezone comparison query fail. Simplified codes.

hiroyuki-sato · 2017-05-19T09:17:24Z

@hito4t Thank you for reviewing.
Please take a look again when you have a time.

hito4t · 2017-05-19T09:33:56Z

@hiroyuki-sato
Thank you!
That's good.
I'll merge it soon.

hiroyuki-sato · 2017-05-19T09:36:34Z

@hito4t Thank you for reviewing.
Do I need to combine multiple commits into a single one?
(Do I need git rebase? )

hito4t · 2017-05-19T09:49:31Z

@hiroyuki-sato
No, I can merge it as it is because it has no conflicts.

hiroyuki-sato · 2017-05-19T09:52:40Z

@hito4t
OK, Thanks!

hiroyuki-sato · 2017-05-26T06:46:09Z

Thank you!

The MySQL plugin compare server and client timezone

434f403

hiroyuki-sato added 2 commits May 3, 2017 23:13

Calculate GMT offset instead of @@system_time_zone

9a3fd65

Output log client timezone name

a30a169

Use offset second instead of offset name

98a2bec

Add MySQLTimeZoneBuilder class for build TimeZone

a8ad95b

hiroyuki-sato added 2 commits May 10, 2017 17:44

Change parameter scope to private

091d65c

Return UTC timezone if offset is zero

14b8b5e

hito4t reviewed May 12, 2017

View reviewed changes

hiroyuki-sato added 2 commits May 12, 2017 22:14

Reimplement compareTimeZone

6ba1f85

Make TimeZoneComparison class

14f82b0

hiroyuki-sato added 3 commits May 17, 2017 15:06

Add error check and change log messages

275dbf3

Change error messages

9a03bcf

Change error message. add period

eb2ae73

Reformat code

c333c7d

hito4t reviewed May 19, 2017

View reviewed changes

hiroyuki-sato added 2 commits May 19, 2017 17:28

Change log level and avoid null check if exception caught.

21adf05

throw SQLException if timezone comparison query fail.

7ca3a9b

hiroyuki-sato changed the title ~~[WIP] The MySQL plugin compare server and client timezone~~ The MySQL plugin compare server and client timezone May 19, 2017

Fix typo

dd2c693

hiroyuki-sato mentioned this pull request May 24, 2017

Compare MySQL server and client time method. embulk/embulk-output-jdbc#174

Merged

hito4t merged commit ced0816 into embulk:master May 26, 2017

hito4t added this to the 0.8.3 milestone May 26, 2017

hiroyuki-sato deleted the mysql_timezone_check branch May 26, 2017 06:46

hiroyuki-sato mentioned this pull request May 26, 2017

[MySQL] set useLegacyDatetimeCode=false by default. #103

Closed

hiroyuki-sato mentioned this pull request Jun 5, 2017

Add mysql ssl option #109

Merged

The MySQL plugin compare server and client timezone #106

The MySQL plugin compare server and client timezone #106

Conversation

hiroyuki-sato commented Apr 29, 2017

Questions

Same timezone

Difference timezone

Compare timezone

hito4t commented May 2, 2017

hiroyuki-sato commented May 2, 2017

hito4t commented May 2, 2017

hiroyuki-sato commented May 2, 2017 • edited Loading

hiroyuki-sato commented May 2, 2017 • edited Loading

hiroyuki-sato commented May 3, 2017

hito4t commented May 10, 2017

hiroyuki-sato commented May 10, 2017

Implementation Idea

Memo

hiroyuki-sato commented May 10, 2017 • edited Loading

hiroyuki-sato commented May 10, 2017 • edited Loading

hito4t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiroyuki-sato commented May 12, 2017

hito4t commented May 16, 2017

hiroyuki-sato commented May 17, 2017 • edited Loading

hito4t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiroyuki-sato commented May 19, 2017

hito4t commented May 19, 2017

hiroyuki-sato commented May 19, 2017

hito4t commented May 19, 2017

hiroyuki-sato commented May 19, 2017

hiroyuki-sato commented May 26, 2017

hiroyuki-sato commented May 2, 2017 •

edited

Loading

hiroyuki-sato commented May 2, 2017 •

edited

Loading

hiroyuki-sato commented May 10, 2017 •

edited

Loading

hiroyuki-sato commented May 10, 2017 •

edited

Loading

hiroyuki-sato commented May 17, 2017 •

edited

Loading