[Scheduled Actions V2] WIP Common/util Package #6903
Conversation
lina-temporal
changed the title
| @@ -0,0 +1,53 @@ | |||
| package common | |||
There was a problem hiding this comment.
Same as the other PR, I'd put everything in the same package.
components/scheduler2/common/util.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // Intended to be called with a substate machine's node. Returns a cloned copy |
There was a problem hiding this comment.
Use Go conventions for docstrings please.
| // Intended to be called with a substate machine's node. Returns a cloned copy | |
| // LoadSchedulerFromParent is intended to be called with a substate machine's node. Returns a cloned copy |
components/scheduler2/util.go
Outdated
|
|
||
| // ValidateTask ensures that the given transition is possible with the current | ||
| // state machine state. | ||
| func ValidateTask[ |
There was a problem hiding this comment.
I think this will generally be applicable but you should consider renaming to something that's more specific. This won't validate any task, just validate that a transition is valid for the given state.
components/scheduler2/util.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // Generates a deterministic request ID for a buffered action's time. The request |
There was a problem hiding this comment.
| // Generates a deterministic request ID for a buffered action's time. The request | |
| // GenerateRequestID generates a deterministic request ID for a buffered action's time. The request |
Also consider not exporting this unless called outside of this package.
There was a problem hiding this comment.
Will fix. Most of these were public access from when I had them in separate packages, I'll give the package a full pass.
components/scheduler2/util.go
Outdated
| // automatically, based on the schedule spec. It must be set for backfills, | ||
| // as backfills may generate buffered actions that overlap with both | ||
| // automatically-buffered actions, as well as other requested backfills. | ||
| func GenerateRequestID(scheduler Scheduler, backfillID string, nominal, actual time.Time) string { |
There was a problem hiding this comment.
Not sure why this needs to be deterministic if you record the intent in the executor's buffer. I would verify that we even support request IDs that aren't UUIDs (we may, I'm just not 100% sure).
One reason that you may want this to be deterministic is to prevent too much diversion during split brain situations.
There was a problem hiding this comment.
Not sure why this needs to be deterministic if you record the intent in the executor's buffer.
I'd made this deterministic primarily before I learned that a timer task will execute while holding a write lock for the whole tree. However, I think it's still useful to avoid a situation where buffered starts are recorded, but the generator's high water mark isn't yet been updated, and the process crashes before that gets to happen (since they are written on separate sub state machines with distinct MachineTransitions). Is that the situation you were referring to with computing next wakeup time instead of caching?
I would verify that we even support request IDs that aren't UUIDs (we may, I'm just not 100% sure).
I'll look into this.
There was a problem hiding this comment.
(since they are written on separate sub state machines with distinct MachineTransitions).
Oops, been a couple weeks since I've had this in my head - since the transaction also encloses the whole timer task, I don't think they really need to be deterministic, but it'd still be helpful for resolution after a split brain situation.
lina-temporal
force-pushed
the
sched2_proto
branch
from
13fc17a to
b4a9fd0
Compare
lina-temporal
force-pushed
the
sched2_common
branch
from
2ce8cb7 to
e403100
Compare
|
Closing, will be merged as part of #6905 |
## What changed? - Added the Generator component, a sub-state machine of the top-level Scheduler - PRs #6904 and #6903 are merged as part of this commit, as they are interdependent: - #6903 - #6904 ## Why? * The generator generates buffered actions, and we want those! ## How did you test it? - New tests, `go test -v`
## What changed? - Added the Generator component, a sub-state machine of the top-level Scheduler - PRs temporalio#6904 and temporalio#6903 are merged as part of this commit, as they are interdependent: - temporalio#6903 - temporalio#6904 ## Why? * The generator generates buffered actions, and we want those! ## How did you test it? - New tests, `go test -v`
Based on #6901
What changed?
Why?
How did you test it?
go test -vPotential risks
Documentation
Is hotfix candidate?