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 UUID primary keys #1131

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Support UUID primary keys #1131

wants to merge 1 commit into from

Conversation

gregpardo
Copy link

The current listing of completed tasks assumes the primary key is an integer.

See relevant issue
#462

This change is intended to support any primary key and adapter.

Instead of getting the latest completed by MAX(id), we instead:

  1. Group the completed tasks by task name and select the latest ended_at (should always be set on completed tasks)
  2. Join back on task_name and latest_ended_at date for the full result

There could be a slight performance impact over the MAX(id) version I believe.

The tests appear to be covered by the tasks_tests.rb

@gregpardo
Copy link
Author

I have signed the CLA!

@gregpardo gregpardo marked this pull request as draft December 11, 2024 14:29
@etiennebarrie
Copy link
Member

Could we keep the existing behavior when not using uuids? There's no index on ended_at, that worries me a bit.

Also just making it clear here, looking at ended_at changes the behavior (probably for the better) if you have a long run, then a short run that finishes first. Currently we'll show the long run while it's still running, then the short run once they're all completed (since it's the last run that's completed). With these changes, we'll show the long run while it's running, and it will still be shown once it's completed.

@gregpardo
Copy link
Author

What about using created_at. It looks like there does appear to be an index on that. Since we are already scoping by completed that would give the latest created that is completed. Seems it would work the same as the existing solution?

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