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

Support arbitrary history task category in SQL persistence #3489

Merged
merged 6 commits into from
Oct 19, 2022

Conversation

yycptt
Copy link
Member

@yycptt yycptt commented Oct 13, 2022

What changed?

  • Support arbitrary history task category in SQL persistence

Why?

  • Currently only cassandra impl support arbitrary task category, need sql support as well since new task queue need to be supported by both cassandra and sql

How did you test it?

  • added unit test

Potential risks

  • N/A. Currently no task category will use those code paths.

Is hotfix candidate?

  • no

@yycptt yycptt requested a review from yiminc October 13, 2022 19:56
Comment on lines +288 to +290
session_start TIMESTAMP DEFAULT '1970-01-02 00:00:01',
last_heartbeat TIMESTAMP DEFAULT '1970-01-02 00:00:01',
record_expiry TIMESTAMP DEFAULT '1970-01-02 00:00:01',
Copy link
Member Author

Choose a reason for hiding this comment

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

@yycptt yycptt marked this pull request as ready for review October 14, 2022 00:15
@yycptt yycptt requested a review from a team as a code owner October 14, 2022 00:15
@yycptt yycptt requested a review from yux0 October 18, 2022 18:51
Comment on lines +79 to +80
`and task_id >= ? ` +
`and task_id < ?`
Copy link
Member

Choose a reason for hiding this comment

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

is this safe?

Copy link
Member Author

@yycptt yycptt Oct 19, 2022

Choose a reason for hiding this comment

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

Yes. This is the correct behavior, all range queries are [inclusive, exclusive).
Didn't discovered this bug before as the code path is never exercised.

Currently we still don't have any queue using this code path (archival queue will be the first one), but at least there're persistence tests ensuring the correct behavior.

@yycptt yycptt force-pushed the sql-arbitrary-task-category branch from 3db2a38 to 1c6a997 Compare October 19, 2022 21:51
@yycptt yycptt merged commit c7713b1 into temporalio:master Oct 19, 2022
@yycptt yycptt deleted the sql-arbitrary-task-category branch October 19, 2022 23:08
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