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

initializes com.mysql.jdbc.TimeUtil.timeZoneMappings if needed (Connector/J 5.x) #259

Merged
merged 3 commits into from
Aug 18, 2024

Conversation

hiroyuki-sato
Copy link
Member

Reviese #200

Accept com.mysql.cj.util.TimeUtil in addition to com.mysql.jdbc.TimeUtil
Connector/J changed TimeUtil class.

@dmikurube
Copy link
Member

As far as I understand, com.mysql.jdbc.TimeUtil should load /com/mysql/jdbc/TimeZoneMapping.properties, and com.mysql.cj.util.TimeUtil should load /com/mysql/cj/util/TimeZoneMapping.properties, respectively, right?

Then, I'd suggest to...

  • Make two more methods, for example, loadTimeZoneMappingsForComMysqlJdbcTimeUtil() for com.mysql.jdbc.TimeUtil, and loadTimeZoneMappingsForComMysqlCjUtilTimeUtil() for com.mysql.cj.util.TimeUtil.
  • Change the existing loadTimeZoneMappings() to call them, such as :
    private void loadTimeZoneMappings() {
        try {
            loadTimeZoneMappingsForComMysqlJdbcTimeUtil();
        } catch (final ClassNotFoundException ex) {
            try {
                loadTimeZoneMappingsForComMysqlCjUtilTimeUtil();
            } catch (final ClassNotFoundException ex2) {
                ex2.addSuppressed(ex);
                throw new RuntimeException(ex2);
            }
        }
    }

@hiroyuki-sato hiroyuki-sato force-pushed the topic/accept-new-timeutil branch from 7e625e3 to 164272d Compare August 18, 2024 02:13
@hiroyuki-sato
Copy link
Member Author

I read the source carefully again.
loadTimeZoneMappings() method was introduced in cb78d86.
I think loadTimeZoneMappings is necessary if the TimeUtil class is com.mysql.jdbc.TimeUtil. (Connector/J 5.x)
Connector/J 8.x and above may not need this method. In fact, this PR works with Connector/J 8.x.

in:
  type: mysql
  host: localhost
  user: user
  password: pasas
  database: embulk_test
  table: test
  driver_path: /tmp/mysql-connector-j-8.4.0/mysql-connector-j-8.4.0.jar
out:
  type: stdout

@hiroyuki-sato hiroyuki-sato changed the title [MySQL] Accept com.mysql.cj.util.TimeUtil in addition to com.mysql.jdbc.TimeUtil Call loadTimeZoneMappings only if the TimeUtil class is com.mysql.jdbc.TimeUtil. Aug 18, 2024
Copy link
Member

@dmikurube dmikurube left a comment

Choose a reason for hiding this comment

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

Basically looks good with a couple of suggestions.

Co-authored-by: Dai MIKURUBE <dmikurube@acm.org>
Copy link
Member

@dmikurube dmikurube left a comment

Choose a reason for hiding this comment

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

LGTM!

@hiroyuki-sato hiroyuki-sato changed the title Call loadTimeZoneMappings only if the TimeUtil class is com.mysql.jdbc.TimeUtil. initializes com.mysql.jdbc.TimeUtil.timeZoneMappings if needed (Connector/J 5.x) Aug 18, 2024
@hiroyuki-sato hiroyuki-sato merged commit 1d173c6 into embulk:master Aug 18, 2024
6 checks passed
@hiroyuki-sato hiroyuki-sato deleted the topic/accept-new-timeutil branch August 18, 2024 12:47
@hiroyuki-sato
Copy link
Member Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants