Skip to content

Add a scheduled_at property to task#367

Open
nsaje wants to merge 4 commits intomasterfrom
add-scheduled-at-prop
Open

Add a scheduled_at property to task#367
nsaje wants to merge 4 commits intomasterfrom
add-scheduled-at-prop

Conversation

@nsaje
Copy link
Contributor

@nsaje nsaje commented Feb 23, 2026

We want to track E2E delay from when a task was scheduled to when it finally ended up being executed. We cannot use the Task.ts because that gets reset to now() every time the task changes state.

@nsaje nsaje requested a review from thomasst February 23, 2026 10:30
Copy link
Member

@thomasst thomasst left a comment

Choose a reason for hiding this comment

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

  • How would you handle update_scheduled_time?
  • Have you considered just using task.ts for this?

@nsaje
Copy link
Contributor Author

nsaje commented Mar 3, 2026

  • How would you handle update_scheduled_time?

Will add handling for it, thanks! If update_scheduled_time is called then scheduled_at should be updated as well - it's intended to convey information about when a task is supposed to run.

  • Have you considered just using task.ts for this?

Yes, that was my initial approach and it's what is used right now for the metric in https://github.com/closeio/closeio/pull/50195 but it's incorrect for this purpose - Task.ts gets changed on every task state change.

@nsaje nsaje requested a review from thomasst March 3, 2026 12:07
)

self._data["scheduled_at"] = ts
tiger.connection.set(tiger._key("task", self.id), json.dumps(self._data))
Copy link
Member

Choose a reason for hiding this comment

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

Should this be part of the pipeline?

The other thing to consider is that if the _data is stale for some reason, it will just overwrite the existing data. I'm not sure if there is a specific case where this would turn into an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it but considered against it - if the Task is not in the queue/SCHEDULED state, we'll still overwrite the data without actually modifying the scheduled time in the queue.

Maybe I can write a Lua script to do all this atomically - I'll give it a go.

@nsaje nsaje requested a review from thomasst March 4, 2026 10:08
@nsaje
Copy link
Contributor Author

nsaje commented Mar 4, 2026

Achieved atomicity with Lua - including patching the _data JSON.

UPDATE_SCHEDULED_TIME = """
local score = redis.call('zscore', KEYS[1], ARGV[2])
if score then
redis.call('zadd', KEYS[1], ARGV[1], ARGV[2])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

zadd mode=XX is not necessary in this script since we check for score upfront

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