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

SAK-48958 Some dates are stored in the database as GMT/UTC with mysql-connector/j 5.1.47+ #11966

Closed
wants to merge 1 commit into from

Conversation

austin48
Copy link
Contributor

@austin48 austin48 commented Oct 4, 2023

No description provided.

# SAK-48958
# When using mysql-connector/j 5.1.47+, configure the timezone, usually your server's localtime
# DEFAULT: GMT
# database.timezone=
Copy link
Contributor

Choose a reason for hiding this comment

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

there is already -Duser.timezone java property. why add another?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is that preferable to sakai.properties? do most institutions run tomcat with that option? (we include it). If so, I can make that change.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it's essential to run Tomcat with the java property if your server uses UTC and your users are in a different timezone

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe an important recommendation has always been to make sure that your tomcat and database configuration are in the same timezone. There have been issues like this that appear when storing dates with no tz info and mysql has changed stances on choosing to store using the mysql servers tz when the jdbc spec says it should be using the (jvm's tz) tz on the connector.

I think keeping Sakai code out of this and using configuration is probably more ideal.

This is the issue that was addressed in 5.1.47 https://bugs.mysql.com/bug.php?id=72609

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to add what I think was the original premise for an OTB setup:

  • setup a database with little to no configuration (no tz customizations) server in same tz as tomcat
  • setup a tomcat with little to no configuration (no tz customizations) server in same tz as database
  • when persisting org.sakaiproject.time.api.Time / java.util.Date / java.time.Instant (all types with tz info)
  • when serializing dates to strings in xml
    The base concept was to store these fields in the database as GMT / UTC 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • without this patch
  • mysql connector 5.1.49
  • tomcat started with -Duser.timezone=Pacific/Honolulu
  • mysql> show variables like "%zone%";
    +------------------+--------+
    | Variable_name | Value |
    +------------------+--------+
    | system_time_zone | HST |
    | time_zone | SYSTEM |
    +------------------+--------+

sakai_event table dates are being saved in GMT, whereas with connector 5.1.46, they are saved in HST timezone. This patch (see PR 11967 for the latest patch) attempts to get it to save in HST timezone with the newer connectors.

Are there other configurations that we should include to keep the sakai code out of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"when serializing dates to strings in xml
The base concept was to store these fields in the database as GMT / UTC 0"

What about when storing dates in date fields (not in xml)?

@@ -419,7 +420,7 @@ public boolean transact(Runnable callback, String tag)
}

/** Used to work with dates in GMT in the db. */
Copy link
Contributor Author

@austin48 austin48 Oct 4, 2023

Choose a reason for hiding this comment

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

@ottenhoff, as the comment states, can you think of anything that requires GMT in the db? Would changing the timezone not be a good idea here? Although it fixed one of my test cases, I wouldn't want to break something else that depends on it being GMT? e.g. does Samigo depend on GMT? I don't know if Samigo would be affected by this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sakai depends on serializing string dates to and from GMT/UTC changing this would likely have consequences.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sakai depends on serializing string dates to and from GMT/UTC changing this would likely have consequences.

Please show one place

@austin48 austin48 closed this Oct 4, 2023
@austin48 austin48 deleted the SAK-48958 branch October 4, 2023 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants