bug: add timestamp to TriggerImmediatelyRequest #606
Conversation
| @@ -278,6 +278,9 @@ message ScheduleState { | |||
| message TriggerImmediatelyRequest { | |||
| // If set, override overlap policy for this one request. | |||
| temporal.api.enums.v1.ScheduleOverlapPolicy overlap_policy = 1; | |||
|
|
|||
| // Timestamp for when this request should be triggered. | |||
There was a problem hiding this comment.
This time is not "when" anything will happen, it will always happen immediately. I'd say it more like:
| // Timestamp for when this request should be triggered. | |
| // Timestamp used for the identity of the target workflow. |
There was a problem hiding this comment.
Even with that explanation I don't understand it exactly. Is this the value that is in TemporalScheduledStartTime SA (and what helps make up the workflow ID) or is this in the workflow start event or something?
There was a problem hiding this comment.
Yeah, it'll be the value in that SA. But more importantly it'll be the value in the workflow id, so the "same" trigger always makes the same workflow id.
Ideally we could fix this issue without touching api, but it does make some sense to have an explicit timestamp here (backfill has explicit timestamps, for example). And putting it here simplifies the server. I don't think it has to be exposed in SDKs but it could be if anyone wants it.
There was a problem hiding this comment.
👍 I suppose my only suggestion would be to call this scheduled_time to clarify it is used as the deterministic/idempotent time the workflow was considered scheduled at, but no strong opinion. I assume we will default this to "now" (also worth clarifying).
There was a problem hiding this comment.
Oh yeah, we should add that it defaults to now in the comment. I'm fine with either name.
_**READ BEFORE MERGING:** All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted._ <!-- Describe what has changed in this PR --> **What changed?** adding a timestamp to `TriggerImmediatelyRequest` message <!-- Tell your future self why have you made these changes --> We can make the wft “more idempotent”: put the desired trigger time in the trigger immediately request itself, so it goes in the signal payload, so all wfts attempt to start the target with the same id. <!-- Are there any breaking changes on binary or code level? --> **Breaking changes** <!-- If this breaks the Server, please provide the Server PR to merge right after this PR was merged. --> [**Server PR**](temporalio/temporal#7968)
READ BEFORE MERGING: All PRs require approval by both Server AND SDK teams before merging! This is why the number of required approvals is "2" and not "1"--two reviewers from the same team is NOT sufficient. If your PR is not approved by someone in BOTH teams, it may be summarily reverted.
What changed?
adding a timestamp to
TriggerImmediatelyRequestmessageWe can make the wft “more idempotent”: put the desired trigger time in the trigger immediately request itself, so it goes in the signal payload, so all wfts attempt to start the target with the same id.
Breaking changes
Server PR