Skip to content

The MySQL plugin compare server and client timezone #106

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.

Sign up for GitHub

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

Merged
merged 16 commits into from
May 26, 2017
Merged

The MySQL plugin compare server and client timezone #106

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
434f403
The MySQL plugin compare server and client timezone
hiroyuki-sato Apr 29, 2017
9a3fd65
Calculate GMT offset instead of @@system_time_zone
hiroyuki-sato May 3, 2017
a30a169
Output log client timezone name
hiroyuki-sato May 3, 2017
98a2bec
Use offset second instead of offset name
hiroyuki-sato May 10, 2017
a8ad95b
Add MySQLTimeZoneBuilder class for build TimeZone
hiroyuki-sato May 10, 2017
091d65c
Change parameter scope to private
hiroyuki-sato May 10, 2017
14b8b5e
Return UTC timezone if offset is zero
hiroyuki-sato May 10, 2017
6ba1f85
Reimplement compareTimeZone
hiroyuki-sato May 12, 2017
14f82b0
Make TimeZoneComparison class
hiroyuki-sato May 12, 2017
275dbf3
Add error check and change log messages
hiroyuki-sato May 17, 2017
9a03bcf
Change error messages
hiroyuki-sato May 17, 2017
eb2ae73
Change error message. add period
hiroyuki-sato May 17, 2017
c333c7d
Reformat code
hiroyuki-sato May 17, 2017
21adf05
Change log level and avoid null check if exception caught.
hiroyuki-sato May 19, 2017
7ca3a9b
throw SQLException if timezone comparison query fail.
hiroyuki-sato May 19, 2017
dd2c693
Fix typo
hiroyuki-sato May 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions embulk-input-jdbc/src/main/java/org/embulk/input/jdbc/AbstractJdbcInputPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,6 +426,8 @@ public TaskReport run(TaskSource taskSource,
LastRecordStore lastRecordStore = null;

try (JdbcInputConnection con = newConnection(task)) {
con.before_load();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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);
    }


List<ColumnGetter> getters = newColumnGetters(con, task, querySchema, pageBuilder);
try (BatchSelect cursor = con.newSelectCursor(builtQuery, getters, task.getFetchRows(), task.getSocketTimeout())) {
while (true) {
Expand Down
7 changes: 7 additions & 0 deletions embulk-input-jdbc/src/main/java/org/embulk/input/jdbc/JdbcInputConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,4 +386,11 @@ private Set<String> getColumnNames(String tableName) throws SQLException
return columnNamesBuilder.build();
}
}

public void before_load()
throws SQLException
{

}

}
49 changes: 49 additions & 0 deletions embulk-input-mysql/src/main/java/org/embulk/input/MySQLTimeZoneBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package org.embulk.input;

import java.sql.Connection;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.Locale;
import java.util.TimeZone;

public class MySQLTimeZoneBuilder
{
private static final int ONE_HOUR_SEC = 3600;
private static final int ONE_MIN_SEC = 60;

public static TimeZone fromSystemTimeZone(Connection connection)
throws SQLException
{
//
// First, I used `@@system_time_zone`. but It return non Time Zone Abbreviations name on a specific platform.
// So, This method calculate GMT offset with query.
//
String query = "select TIME_TO_SEC(timediff(now(),utc_timestamp()));";
Statement stmt = connection.createStatement();
ResultSet rs = stmt.executeQuery(query);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Add stmt.close exception handling is still TODO.


if (rs.next()) {
int offset_seconds = rs.getInt(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Variable names use camel case like offsetSeconds.

return fromGMTOffsetSeconds(offset_seconds);
}
else {
// TODO Error check.
return null;
}
}

public static TimeZone fromGMTOffsetSeconds(int offset_seconds)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to be public?

Copy link
Member Author

Choose a reason for hiding this comment

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

Change to private.

{
if( offset_seconds == 0 ) {
return TimeZone.getTimeZone("UTC");
}

String sign = offset_seconds > 0 ? "+" : "-";
int abs_offset_sec = Math.abs(offset_seconds);
int tz_hour = abs_offset_sec / ONE_HOUR_SEC;
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);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it work for summer time?

Copy link
Member Author

Choose a reason for hiding this comment

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

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();
        }

}
30 changes: 30 additions & 0 deletions embulk-input-mysql/src/main/java/org/embulk/input/mysql/MySQLInputConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
package org.embulk.input.mysql;

import java.sql.Statement;
import java.util.List;
import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.SQLException;
import java.sql.ResultSet;
import java.util.Locale;
import java.util.TimeZone;

import com.mysql.jdbc.ConnectionImpl;
import com.mysql.jdbc.ConnectionProperties;
import org.embulk.input.MySQLTimeZoneBuilder;
import org.embulk.input.jdbc.JdbcInputConnection;
import org.embulk.input.jdbc.JdbcLiteral;
import org.embulk.input.jdbc.getter.ColumnGetter;
Expand Down Expand Up @@ -59,4 +62,31 @@ public TimeZone getServerTimezoneTZ()
{
return ((ConnectionImpl) connection).getServerTimezoneTZ();
}

@Override
public void before_load()
throws SQLException
{
// TODO error check.
TimeZone svr_tz = MySQLTimeZoneBuilder.fromSystemTimeZone(connection);

String usr_tz_name = System.getProperty("user.timezone");
TimeZone usr_tz = TimeZone.getTimeZone(usr_tz_name);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to TimeZone.getDefaultValue.


//
// Compare offset only. Although I expect to return true, the following code return false,
//
// TimeZone tz_jst = TimeZone.getTimeZone("JST");
// TimeZone tz_gmt9 = TimeZone.getTimeZone("GMT+9");
// tz_jst.hasSameRules(tz_gmt9) // return false.
//
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));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

logger.warn(String.format(Locale.ENGLISH,
"Use `options: { useLegacyDatetimeCode: false }`"));
}
logger.warn(String.format(Locale.ENGLISH,"The plugin will set `useLegacyDatetimeCode=false` by default in future."));
}

}