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

Fix time fields of rows from GetClusterMembers #3840

Merged

Conversation

rodrigozhou
Copy link
Contributor

What changed?
In GetClusterMembers, fix the time fields, ie., call date time converter.

Why?
The times are stored in UTC in DB, but when reading, the time zone might not be set correctly. Calling the date time converter fixes it.

How did you test it?
Existing tests.

Potential risks
None.

Is hotfix candidate?
No.

@rodrigozhou rodrigozhou requested a review from a team as a code owner January 25, 2023 06:16
@rodrigozhou rodrigozhou force-pushed the fix-cluster-metadata-time-fields branch from 285a757 to 0da79ec Compare January 25, 2023 19:48
Copy link
Member

@yycptt yycptt left a comment

Choose a reason for hiding this comment

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

Can we add a test for to prevent future regression?

@rodrigozhou
Copy link
Contributor Author

@yycptt I found this issue because I'm making a change in the SQLite driver connection config and unit tests failed.

In practice, those conversions calls are unnecessary because the date time are already stored in UTC. However, I'm changing a config related to the date time format to store in SQLite to ISO format, and go time.Parse doesn't translate "+00:00" to the internal time.UTC location, which caused the test to fail.

Currently, it's writing in the format "2006-01-02 15:04:05.999999999 -0700 MST", ie., it contains the 3-letter timezone code. In our case, it contains the "UTC" string.

I'm changing to the format "2006-01-02 15:04:05.999999999-07:00", and because it doesn't contain the 3-letter timezone code, time.Parse doesn't set the time zone as time.UTC location, but as time zone location equivalent to it (ie., +00:00).

@rodrigozhou rodrigozhou merged commit 5652dc4 into temporalio:master Jan 26, 2023
@rodrigozhou rodrigozhou deleted the fix-cluster-metadata-time-fields branch January 26, 2023 01:18
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.

2 participants