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.

Sign up for GitHub

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

Jump to bottom

Scheduled jobs with unique #715

Closed
camaeel opened this issue Jan 1, 2025 · 3 comments · Fixed by #734
Closed

Scheduled jobs with unique #715

camaeel opened this issue Jan 1, 2025 · 3 comments · Fixed by #734
Assignees
@brandur

Comments

@camaeel
Copy link

camaeel commented Jan 1, 2025

I'm dynamically inserting jobs into river with ScheduledAt attribute.

It seems from my tests that when I call:

res, err := riverClient.Insert(ctx,
	SomeArgs{}, &river.InsertOpts{
		ScheduledAt: scheduleTime,
		UniqueOpts: river.UniqueOpts{
			ByPeriod: 12 * time.Minute,
		},
	},
)

twice in a loop, second job is treated as violating uniqueness condition and not created. scheduleTime is (of course) different by more than 12 minutes.

Does "Uniqueness" feature check only created time, and dosen't work well with scheduled?
@brandur
Copy link
Contributor

brandur commented Jan 4, 2025

Yeah, it does indeed look like this'll always used the current time. I think it should work to alternatively use scheduled_at. I'll take a crack at it.

@brandur brandur self-assigned this Jan 4, 2025
brandur added a commit that referenced this issue Jan 25, 2025
@brandur
Allow by period uniqueness to be based off scheduled time
b5e67e3 
This one attempts to resolve #715. Currently, by period uniqueness
always bases the period off the current time, but there's a good
argument that if the job has been scheduled for a particular time in the
future, it should be based off that time instead.

This is one that could nominally be considered a small breaking change
in a Hyrum's Law sort of way, even though it's really patching what
could be considered a bug. Even though it was sort of broken before,
some apps may have come to depend on the broken behavior of the unique
code ignoring `ScheduledAt`. I'm not sure that it's a big enough problem
to be worth calling out though, so I didn't.

Fixes #715.
@brandur brandur mentioned this issue Jan 25, 2025
Merged
brandur added a commit that referenced this issue Jan 25, 2025
@brandur
Allow by period uniqueness to be based off scheduled time
a672ad3 
This one attempts to resolve #715. Currently, by period uniqueness
always bases the period off the current time, but there's a good
argument that if the job has been scheduled for a particular time in the
future, it should be based off that time instead.

This is one that could nominally be considered a small breaking change
in a Hyrum's Law sort of way, even though it's really patching what
could be considered a bug. Even though it was sort of broken before,
some apps may have come to depend on the broken behavior of the unique
code ignoring `ScheduledAt`. I'm not sure that it's a big enough problem
to be worth calling out though, so I didn't.

Fixes #715.
@brandur
Copy link
Contributor

brandur commented Jan 25, 2025

Posted a fix for this in #734. It'll go out with the next release.

@camaeel
Copy link
Author

camaeel commented Jan 25, 2025

@brandur Great, thanx. I will try it when it is released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@brandur brandur

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow by period uniqueness to be based off scheduled time riverqueue/river
2 participants
@brandur @camaeel